Skip to content

Add channel mask to LoraWan constructor and expose LoraWan modem getters #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

ilcato
Copy link
Contributor

@ilcato ilcato commented Oct 18, 2021

Enable setting sub band frequency into LoraWan Connection handler constructor.
Expose LoraWan modem getters.

@github-actions
Copy link

Memory usage change @ 45f17ab

Board flash % RAM for global variables %
arduino:mbed:envie_m4 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed:envie_m7 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nanorp2040connect 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkr1000 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrgsm1400 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrnb1500 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrwan1300 🔺 +320 - +320 +0.12 - +0.12 0 - 0 0.0 - 0.0
arduino:samd:mkrwan1310 🔺 +320 - +320 +0.12 - +0.12 0 - 0 0.0 - 0.0
arduino:samd:mkrwifi1010 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:nano_33_iot 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
esp32:esp32:esp32 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
esp8266:esp8266:huzzah 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table
Board examples/ConnectionHandlerDemo
flash
% examples/ConnectionHandlerDemo
RAM for global variables
%
arduino:mbed:envie_m4 0 0.0 0 0.0
arduino:mbed:envie_m7 0 0.0 0 0.0
arduino:mbed_nano:nanorp2040connect 0 0.0 0 0.0
arduino:samd:mkr1000 0 0.0 0 0.0
arduino:samd:mkrgsm1400 0 0.0 0 0.0
arduino:samd:mkrnb1500 0 0.0 0 0.0
arduino:samd:mkrwan1300 320 0.12 0 0.0
arduino:samd:mkrwan1310 320 0.12 0 0.0
arduino:samd:mkrwifi1010 0 0.0 0 0.0
arduino:samd:nano_33_iot 0 0.0 0 0.0
esp32:esp32:esp32 0 0.0 0 0.0
esp8266:esp8266:huzzah 0 0.0 0 0.0
Click for full report CSV
Board,examples/ConnectionHandlerDemo<br>flash,%,examples/ConnectionHandlerDemo<br>RAM for global variables,%
arduino:mbed:envie_m4,0,0.0,0,0.0
arduino:mbed:envie_m7,0,0.0,0,0.0
arduino:mbed_nano:nanorp2040connect,0,0.0,0,0.0
arduino:samd:mkr1000,0,0.0,0,0.0
arduino:samd:mkrgsm1400,0,0.0,0,0.0
arduino:samd:mkrnb1500,0,0.0,0,0.0
arduino:samd:mkrwan1300,320,0.12,0,0.0
arduino:samd:mkrwan1310,320,0.12,0,0.0
arduino:samd:mkrwifi1010,0,0.0,0,0.0
arduino:samd:nano_33_iot,0,0.0,0,0.0
esp32:esp32:esp32,0,0.0,0,0.0
esp8266:esp8266:huzzah,0,0.0,0,0.0

@per1234 per1234 added the type: enhancement Proposed improvement label Oct 19, 2021
@stevisco stevisco requested review from pennam and aentinger October 19, 2021 09:18
@stevisco
Copy link

@aentinger @pennam can you please review ?

Copy link
Contributor

@pennam pennam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to add a specific enum for subfrequency band here https://github.com/arduino-libraries/MKRWAN/blob/5cc2f53395ffd7fdaabc213471dc08d37f2aa96c/src/MKRWAN.h#L262-L267 to be used as function argument

@stevisco stevisco requested a review from facchinm October 22, 2021 09:21
@ilcato ilcato changed the title Add sub frequency param to LoraWan constructor and expose LoraWan modem getters Add channel mask to LoraWan constructor and expose LoraWan modem getters Oct 23, 2021
@github-actions
Copy link

Memory usage change @ 832f180

Board flash % RAM for global variables %
arduino:mbed:envie_m4 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed:envie_m7 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:mbed_nano:nanorp2040connect 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkr1000 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrgsm1400 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrnb1500 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:mkrwan1300 🔺 +232 - +232 +0.09 - +0.09 🔺 +8 - +8 +0.02 - +0.02
arduino:samd:mkrwan1310 🔺 +232 - +232 +0.09 - +0.09 🔺 +8 - +8 +0.02 - +0.02
arduino:samd:mkrwifi1010 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
arduino:samd:nano_33_iot 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
esp32:esp32:esp32 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
esp8266:esp8266:huzzah 0 - 0 0.0 - 0.0 0 - 0 0.0 - 0.0
Click for full report table
Board examples/ConnectionHandlerDemo
flash
% examples/ConnectionHandlerDemo
RAM for global variables
%
arduino:mbed:envie_m4 0 0.0 0 0.0
arduino:mbed:envie_m7 0 0.0 0 0.0
arduino:mbed_nano:nanorp2040connect 0 0.0 0 0.0
arduino:samd:mkr1000 0 0.0 0 0.0
arduino:samd:mkrgsm1400 0 0.0 0 0.0
arduino:samd:mkrnb1500 0 0.0 0 0.0
arduino:samd:mkrwan1300 232 0.09 8 0.02
arduino:samd:mkrwan1310 232 0.09 8 0.02
arduino:samd:mkrwifi1010 0 0.0 0 0.0
arduino:samd:nano_33_iot 0 0.0 0 0.0
esp32:esp32:esp32 0 0.0 0 0.0
esp8266:esp8266:huzzah 0 0.0 0 0.0
Click for full report CSV
Board,examples/ConnectionHandlerDemo<br>flash,%,examples/ConnectionHandlerDemo<br>RAM for global variables,%
arduino:mbed:envie_m4,0,0.0,0,0.0
arduino:mbed:envie_m7,0,0.0,0,0.0
arduino:mbed_nano:nanorp2040connect,0,0.0,0,0.0
arduino:samd:mkr1000,0,0.0,0,0.0
arduino:samd:mkrgsm1400,0,0.0,0,0.0
arduino:samd:mkrnb1500,0,0.0,0,0.0
arduino:samd:mkrwan1300,232,0.09,8,0.02
arduino:samd:mkrwan1310,232,0.09,8,0.02
arduino:samd:mkrwifi1010,0,0.0,0,0.0
arduino:samd:nano_33_iot,0,0.0,0,0.0
esp32:esp32:esp32,0,0.0,0,0.0
esp8266:esp8266:huzzah,0,0.0,0,0.0

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Please have a look at the comments for details.

One general question: Do we really need all those get-methods? In general I prefer to keep any API as small as possible without compromising functionality. Looks to me like all those getters mainly serve debugging purposes during development?

@facchinm facchinm merged commit a0f656f into arduino-libraries:master Nov 2, 2021
@ilcato ilcato deleted the add-sub-frequency-param-to-lorawan-constructor branch November 2, 2021 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants