Skip to content
New issue

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

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

Already on GitHub? # to your account

Parse JoinAccept (Partial) #23

Closed
wants to merge 4 commits into from

Conversation

Oliv4945
Copy link
Contributor

Parse Delay and DLsettings->RX2 Data rate, tested thanks to TTN

Does not parse DLsettings->RX1DRoffset as it seems that no mechanism for that is implemented yet.
I think that adding a new field to lmic_t is the better way but maybe you have another idea less RAM consumming ?
Neverheless we should implement it for full LoraWAN compliance.

@matthijskooijman
Copy link
Owner

Thanks for this PR, the code looks good to me. Some remarks regarding the form, though:

  • You have two commits for the same thing now, seems the second one fixes a mistake in the first one. If this happens, you should either write the commit message of the second commit appropriately (explaining what it fixes), or (preferred by me) just modify the original commit to include the fix (using git commit --amend in this case, or using git rebase (see also https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History).
  • Additionally, your commit(s) contain 3 distinct logical changes: Modify the code to allow a variable RX delay, read the RX delay from join accept and read dn2dr from join accept. I'd like to see these separated (possibly in three different commits, but putting either the first two changes or the last two changes in the same commit also seems sensible to me). As it is now, the size of the first change makes it harder to see the latter two changes.

Let me know if you need help figuring out how to approach this :-)

@Oliv4945 Oliv4945 force-pushed the joinAcceptProcess branch from d223ffe to 6bd46e3 Compare June 23, 2016 21:43
@Oliv4945
Copy link
Contributor Author

Oliv4945 commented Jun 23, 2016

Ok, I am not enough used to those git commands but a friend of mine help, does it works for you know ?
Sorry for the dirty initial commit, I will pay more attention next time !

Ref issue #20 as it partially answers to it.

@matthijskooijman
Copy link
Owner

Looks perfect like this, thanks! I'll try to do some testing and merge this next week.

Was not working if ping is disabled
@Oliv4945 Oliv4945 force-pushed the joinAcceptProcess branch from b0eb861 to 686cec2 Compare July 12, 2016 22:30
@matthijskooijman
Copy link
Owner

IBM is intending to change the license on the LMIC library to the three-clause BSD license. To simplify incorporating your contribution upstream on the next LMIC release, would you be willing to license it under the same license? If so, please explicitly state this in a comment to this PR? Saying something like "This contribution is licensed under the terms of the three-clause BSD license, as linked above" should be sufficient.

@hallard
Copy link

hallard commented Aug 8, 2016

This contribution is licensed under the terms of the three-clause BSD license, as linked above

@Oliv4945
Copy link
Contributor Author

Oliv4945 commented Aug 8, 2016

This contribution is licensed under the terms of the three-clause BSD license, as linked above.

@matthijskooijman
Copy link
Owner

I rebased your commits, merged the last one into the first one and manually merged it. Thanks!

matthijskooijman referenced this pull request Aug 10, 2016
This is very similar to the ttn-abp example, except that it uses OTAA
for setting up a session instead of setting it up statically.
@Oliv4945 Oliv4945 deleted the joinAcceptProcess branch December 12, 2016 21:49
@avbentem
Copy link

Am I right to assume that "When using OTAA [...] you should manually set the RX2 rate, after joining" in README.md is outdated?

When using OTAA, the network communicates the RX2 settings in the join accept message, but the LMIC library does not currently process these settings. Until that is solved (see issue #20), you should manually set the RX2 rate, after joining (see the handling of EV_JOINED in the ttn-otaa.ino for an example.

If not, then it's missing a closing bracket ;-)

Currently that example shows:

   case EV_JOINED:
        Serial.println(F("EV_JOINED"));

        // Disable link check validation (automatically enabled
        // during join, but not supported by TTN at this time).
        LMIC_setLinkCheckMode(0);
        break;

...which is unrelated to the RX2 rate.

@Oliv4945
Copy link
Contributor Author

Am I right to assume that "When using OTAA [...] you should manually set the RX2 rate, after joining" in README.md is outdated?
Yes you are right, RX2 rate is know set by the library

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants