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

Fix coding of the "Margin" field of the "DevStatusAns" command payload #131

Merged
merged 9 commits into from
Sep 27, 2018

Conversation

frazar
Copy link

@frazar frazar commented Sep 24, 2018

The value of 31 is taken from this line of the lorawan software.

Fixes #130

@terrillmoore terrillmoore self-requested a review September 24, 2018 21:45
Copy link
Member

@terrillmoore terrillmoore left a comment

Choose a reason for hiding this comment

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

Thank you again for finding and reporting this! Let me know how best to collaborate on this. I can potentially send you a patch set to test, or you can make my changes from the comment.

src/lmic/lmic.c Outdated Show resolved Hide resolved
@terrillmoore
Copy link
Member

Comments in-line below.

Thank you for your writeup!
Your fix seems definitely like the best way to address the problem.

I have a couple of questions before I implement these changes:

1. Regarding the following lines,
   ```
   // LMIC.snr is SNR time 4, convert to real SNR; rounding towards zero.
   const int snr = (LMIC.snr + 2) / 4;
   ```
   Why is the `LMIC.snr` value incremented by 2? 

This rounds the value. If LMIC.snr is 0 or 1, (LMIC.snr4)+2 is 2 or 3; if 2 or 3, then (LMICsnr*4)+2 is 4 or 5. Thus LMIC.snr values map as follows:

LMIC.snr represents rounded value
0, 1 0.0, 0.25 0
1, 2, 3 ,4 0.5, 0.75, 1.0, 1.25 1

and so forth.

And should the snr variable be declared as s2_t rather than int?

int is normally more efficient computationally. Some processors actually work harder to maintain exact C semantics if you declare something narrower than you need. I think on AVR this will be 16 bits and on ARM it will be 32 bits. There's no benefit to s2_t on ARM. If the type system LMIC uses had s2_fast_t, that would be the right declaration (as that's "at least 16 bits" not "exactly 16 bits").

2. Regarding the following change,
   ```
   LMIC.frame[end+3] = LMIC.devAnsMargin;
   ```
   Shouldn't the `LMIC.devAnsMargin` be in the 2nd byte of the `DevStatusAns` Payload rather than the 3rd?
   If not, then should the `end += 3;` at line 1267 be changed to `end += 4`?
   Maybe I'm just confused about how the `LMIC.frame` works..

Nope, I screwed up. Should have written LMIC.frame[end+2] = LMIC.devAnsMargin; to replace the existing LMIC.frame[end+2] = LMIC.margin; Thanks for catching this.

3. In your comments you refer to `[1.02]` or `[1.1]`, but I think the definition of margin has been the same [since v1.0](https://lora-alliance.org/resource-hub/lorawantm-specification-v10), so reporting the revision number is unnecessary.

Yeah, I never check the older specs, so I just referenced what I have. Use your judgment.

Copy link
Member

@terrillmoore terrillmoore 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 we still need the operative change at around line 1267 (use the devAnsValue in the response).

src/lmic/lmic.h Outdated Show resolved Hide resolved
@frazar
Copy link
Author

frazar commented Sep 26, 2018

Thank you very much for your extensive commments and explanations!

@frazar frazar changed the title Change the maxium allowed value of margin from 254 to 31 Fix coding of the "Margin" field of the "DevStatusAns" command payload Sep 26, 2018
@terrillmoore
Copy link
Member

Thanks for finding and fixing this! Changes look good to me, will merge now.

@terrillmoore terrillmoore merged commit d9225fb into mcci-catena:master Sep 27, 2018
@frazar frazar deleted the set_max_margin_to_31 branch October 2, 2018 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants