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

Add payload MAC commands parsing + LinkcheckReq/Ans MAC commands #19

Closed
wants to merge 3 commits into from

Conversation

Oliv4945
Copy link
Contributor

  • Modified MAC parsing in order to allow MAC command in header or piggybacked
  • Modified default RX2 to SF9.
    • It is not default value but this library is mostly used with TTN backend, so I think it will help new users
  • Added LMIC_setLinkCheckRequestOnce to get info on gateway number and margin from the backend
    • The name might be confusing with LMIC developpers' choice for "LMIC_setLinkCheckMode" but I prefer to avoid changing it for easier adaptation to futur LMIC versions

All modifications are commented with Changes to LMIC v1.5 in order to help upgrades to LMIC v1.6, if any.

@@ -1127,7 +1031,7 @@ static bit_t decodeFrame (void) {
}
case MCMD_DEVS_REQ: {
LMIC.devsAns = 1;
oidx += 1;
oidx += 1;
Copy link
Owner

Choose a reason for hiding this comment

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

This is just a pointless whitespace change, please remove it.

@matthijskooijman
Copy link
Owner

Thanks for your PR! However, you've put a few different things in a single commit, whereas I'd rather have different commits for different (logical) changes.

Modified MAC parsing in order to allow MAC command in header or piggybacked

Didn't realize that LMIC did not support this yet, so great change to have. The diff is a bit more invasive then I'd like, but I guess that's not to be helped when splitting a big function like decodeFrame() (and doing so indeed seems the best way to approach this).

Note that the commit message says you added support for piggybacked MAC commands, but AFAICS you actually added support for dedicated MAC command packets, MAC commands piggybacked on regular packets were already supported, right?

Modified default RX2 to SF9

This change makes the library non-standards compliant, so I'd rather not add this. There is already a note about this for the semtech backend in the README, I'll add a note about TTN too. Also, the ttn.ino example should probably default to SF9 in the sketch code. I have some other changes to the sketch code pending, so I'll make that change myself - just leave it out of this PR.

Note that this only helps for personalized activation - for OTAA we should really fix LMIC to let its processJoinAccept() function process the DLsettings and RxDelay fields in the join accept message.

Added LMIC_setLinkCheckRequestOnce to get info on gateway number and margin from the backend

This change looks good to me. It's a pity it needs to add a few more bytes of static RAM usage, but I guess that's not to be helped with the way LMIC is designed now. Good to have this feature as well - I was thinking of implementing a node-side ADR strategy (since TTN doesn't do server-side ADR yet), and this would help. Any idea if TTN replies to these requests already?

All modifications are commented with Changes to LMIC v1.5 in order to help upgrades to LMIC v1.6, if any.

This shouldn't be needed, since git already tracks what changed adequately (in my experience, such comments often get in the way of future changes instead of really helping).

Again, thanks for your contribution, the code looks good. If you could split off the first and last changes, and drop the second one, that would be perfect. Note that you can update this PR by doing a force-push to the branch you created this PR from.

@Oliv4945
Copy link
Contributor Author

Oliv4945 commented Jun 16, 2016

but AFAICS you actually added support for dedicated MAC command packets, MAC commands piggybacked on regular packets were already supported, right?

True, I might have been a bit fast this morning

should probably default to SF9 in the sketch code. I have some other changes to the sketch code pending, so I'll make that change myself - just leave it out of this PR

Ok

for OTAA we should really fix LMIC to let its processJoinAccept() function process the DLsettings and RxDelay fields in the join accept message.

Yes it is something I want to do, but I need to understand before why OTAA works on 16MHz and 8MHz ATMega. I will open an issue for this discussion.

It's a pity it needs to add a few more bytes of static RAM usage, but I guess that's not to be helped with the way LMIC is designed now.

I agree with you, but I do not see an other way :/

and this would help. Any idea if TTN replies to these requests already?

I have no answer from TTN, although it works with 2 other operators.

This shouldn't be needed,
Ok, I will delete them

Again, thanks for your contribution, the code looks good. If you could split off the first and last changes, and drop the second one, that would be perfect. Note that you can update this PR by doing a force-push to the branch you created this PR from

You are welcome, you already did an amazing job ! I do not master push request so I'll check how to do it.

@matthijskooijman
Copy link
Owner

Yes it is something I want to do, but I need to understand before why OTAA works on 16MHz and 8MHz ATMega. I will open an issue for this discussion.

Not sure what you mean by that, but I'm sure you'll explain in the new issue you're going to open :-)

for OTAA we should really fix LMIC to let its processJoinAccept() function process the DLsettings and RxDelay fields in the join accept message.

I opened #20 to track this issue separately.

@Oliv4945 Oliv4945 changed the title Add piggyback MAC parsing + LinkcheckReq + RX2=SF9 Add payload MAC commands parsing + LinkcheckReq/Ans MAC commands Jun 25, 2016
@Oliv4945
Copy link
Contributor Author

Ok, it should be good know, hope I didn't messed up anything.
Does it works for you ? Now I understand the way you want to split PR, it should be better !

@matthijskooijman
Copy link
Owner

From a quick glance, it looks good. I'll have a closer look when I have some more time, but thanks already!

@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.

The name might be confusing with LMIC developpers' choice for "LMIC_setLinkCheckMode" but I prefer to avoid changing it for easier adaptation to futur LMIC versions.
@Oliv4945
Copy link
Contributor Author

Oliv4945 commented Nov 2, 2016

Hi Matthijs, if I not messed up with Git, the PR should be updated to your last commit.
Is it ok for you ?

@gizmocuz
Copy link

+1

@ghost
Copy link

ghost commented Jul 14, 2017

Hello!

I using LoRaWan Node with Pro mini and RFM96.

And When I run code

Starting
Packet queued
214: EV_JOINING

And I waiting, but not change.

@matthijskooijman Can you help me?

@matthijskooijman
Copy link
Owner

@VanXungUIT, posting to a random, unrelated, github pull request is not the way to ask for help. More generally, github issues are meant for bugs and feature requests, not help with your code. I suggest you make a post on the TTN forum, which is probably the best platform for support right now.

@Jeroen88
Copy link

@matthijskooijman Hi Matthijs, any progress on this PR? I see that KPN is sending MAC commands as payload at FPort = 0. If I understand it well, these MACs are currently being ignored by the LMiC library. One of @Oliv4945's commits does implement this, doesn't it? Best, Jeroen

@matthijskooijman
Copy link
Owner

This repository is now deprecated, see #297 for some more background. I'm grateful for your contribution, but it will no longer be merged. I'm recommending people to use the MCCI version of LMIC instead. If this PR addresses an issue that also exists in that version, I would encourage you to resubmit your contribution there, so it might benefit other users. I'm sorry for the inconvenience...

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.

5 participants