Skip to content
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

lorawan: make possible to send empty frames #72600

Merged
merged 1 commit into from May 12, 2024

Conversation

aurel32
Copy link
Collaborator

@aurel32 aurel32 commented May 10, 2024

Empty frames are allowed by the LoRaWAN protocol and are actually useful to open new RX slots or flush MAC commands in stack. Therefore allow the data pointer to be NULL if len is 0.

Note that I am not even sure we want to have this check. It seems in other subsystems, it is the responsibility of the caller to ensure the data pointer is not NULL.

@zephyrbot zephyrbot added Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. area: LoRa labels May 10, 2024
Copy link
Member

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

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

I'm fine with that change, only got a small suggestion for improvement.

I'd leave the NULL check though, unless you have double-checked that the LoRaWAN will definitely fail gracefully even for data == NULL && len > 0.

@@ -595,7 +595,7 @@ int lorawan_send(uint8_t port, uint8_t *data, uint8_t len,
int ret = 0;
bool empty_frame = false;

if (data == NULL) {
if (len && data == NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to make the len check more explicit and swap the conditions, so that the second condition doesn't have to be evaluated in most cases:

Suggested change
if (len && data == NULL) {
if (data == NULL && len > 0) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I have just updated my PR.

@aurel32
Copy link
Collaborator Author

aurel32 commented May 10, 2024

I'm fine with that change, only got a small suggestion for improvement.

I'd leave the NULL check though, unless you have double-checked that the LoRaWAN will definitely fail gracefully even for data == NULL && len > 0.

The loramac-node code ignores len if data is NULL, so it will not fail: https://github.com/zephyrproject-rtos/loramac-node/blob/master/src/mac/LoRaMac.c#L3302

Empty frames are allowed by the LoRaWAN protocol and are actually useful
to open new RX slots or flush MAC commands in stack. Therefore allow the
data pointer to be NULL if len is 0.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
@aurel32 aurel32 force-pushed the lorawan-send-empty-frames branch from e76ee0b to 1a5e412 Compare May 10, 2024 21:50
Copy link
Collaborator

@JordanYates JordanYates left a comment

Choose a reason for hiding this comment

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

QoL change, you can already send a 0 length packet by proving a non-NULL pointer with a length of 0.

@nashif nashif merged commit a6fbfc0 into zephyrproject-rtos:main May 12, 2024
21 checks passed
@aurel32 aurel32 deleted the lorawan-send-empty-frames branch May 12, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: LoRa Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants