mgmt: mcumgr: transport: Add LoRaWAN MCUmgr SMP transport #80106
Conversation
zephyrbot
requested review from
dkalowsk,
kartben,
mmahadevan108 and
nordicjm
thedjnK
force-pushed
the
mcumgrlora
branch
2 times, most recently
from
cf92709 to
2dd4473
Compare
kartben
left a comment
kartben
left a comment
There was a problem hiding this comment.
Very cool!
This needs a global search and replace s/LoRa/LoRaWAN/g though as it actually is a LoRaWAN transport, not "LoRa" or "LoRa (LoRaWAN)" one. Related, referring to things like e.g. "LoRa frame port" is incorrect.
Renamed |
thedjnK
changed the title
There was a problem hiding this comment.
If data_size > size, then data = NULL then the lorawan_send will return -EINVAL, this will be counted as -- to 'tries', that is correct yes?
There was a problem hiding this comment.
Spotted a mistake where the final packet might have been too large. data will only be null if there is nothing to transmit (which in LoRaWAN is allowed, there is still a transmission but the optional payload is not present), if a message is queued for sending then it will send the full thing chunk by chunk
de-nordic
left a comment
de-nordic
left a comment
There was a problem hiding this comment.
One stupid question, otherwise looks ok.
There was a problem hiding this comment.
for my own education, why ; here? Or maybe it's a typo?
There was a problem hiding this comment.
Typo, should be ,, fixed
kartben
dismissed
their stale review
dismiss as my comment re: renaming LoRA --> LoRaWAN has been addressed. Thanks!
There was a problem hiding this comment.
| Enables handling of SMP commands received over LoRa. | |
| Enables handling of SMP commands received over LoRaWAN. |
There was a problem hiding this comment.
Port 0 is not allowed for applications, it's reserved for MAC commands. The allowed range for applications is 1-223, see here.
| range 0 120 | |
| range 1 223 |
There was a problem hiding this comment.
Suggestion to clarify that we are talking about uplinks:
| config MCUMGR_TRANSPORT_LORAWAN_CONFIRMED_PACKETS | |
| config MCUMGR_TRANSPORT_LORAWAN_CONFIRMED_UPLINKS |
There was a problem hiding this comment.
There is an FPending flag for downlink messages for exactly this purpose. Can we not use that instead?
Quote from the spec:
4.3.1.4 Frame pending bit (FPending in FCtrl, downlink only)
The frame pending bit (FPending) is only used in downlink communication, indicating that
the network has more data pending to be sent and therefore asking the end-device to open
another receive window as soon as possible by sending another uplink message.
There was a problem hiding this comment.
I don't see how that would work any differently? If the flag is set, you still all the same code, the way it is right now means it only sends an empty frame if there is outstanding data for the current MCUmgr packet which in my mind is all this should handle, something non-MCUmgr can be retrieved by the application code
There was a problem hiding this comment.
I thought the LoRaWAN stack would handle the FPending flag automatically, but it doesn't seem to be the case. I agree your implementation makes sense.
There was a problem hiding this comment.
This would probably please the clang-format check.
| .cb = smp_lorawan_downlink | |
| .cb = smp_lorawan_downlink, |
There was a problem hiding this comment.
clang-format is completely broken in most checks which is why it does not fail CI, the suggestion from clang-format here is + .port = CONFIG_MCUMGR_TRANSPORT_LORAWAN_FRAME_PORT, .cb = smp_lorawan_downlink}; which looks ghastly. Later on it suggest this + K_KERNEL_STACK_SIZEOF(smp_lorawan_stack), smp_lorawan_uplink_thread, NULL, which is funny because the suggestion would be a CI failure as that line is 102 characters
There was a problem hiding this comment.
Sending confirmed uplinks is discouraged by TheThingsNetwork as the gateway downlink traffic bandwidth is very limited. Should we use unconfirmed messages by default?
There was a problem hiding this comment.
This only applies to the (public) things network because they're trying to get you to pay for their commercial offering (the things industries), it does not have any relation to anything in the spec or LoRa in general
Adds a transport that uses LoRaWAN for receiving and responding to messages Signed-off-by: Jamie McCrae <[email protected]>
Adds a test to ensure that the LoRaWAN MCUmgr transport builds with all options enabled and all options disabled Signed-off-by: Jamie McCrae <[email protected]>
Adds a note that this transport has been added Signed-off-by: Jamie McCrae <[email protected]>
6 participants
Adds a transport that uses LoRaWAN for receiving and responding to messages
Showing transport in action:
