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

Honor the channel mask while trying to join the network. #45

Conversation

frankleonrose
Copy link
Contributor

Also fixes a bug where nextJoinState() chooses unrelated 500kHz channel rather than the one associated with the current channel's sub-band.

@jcwren
Copy link

jcwren commented Apr 21, 2017

Can you check your logic on line 886 (left-hand line number column), please? I believe it's inverted from the comments. After applying your patch and watching the channel frequencies as it's trying to join, it's using frequencies outside the enabled sub-band.

@frankleonrose
Copy link
Contributor Author

frankleonrose commented Apr 21, 2017

@jcwren [edited] After asking @terrillmoore, the code looks correct as it stands. If last transmit we were not using DR_SF8C, we select a 500kHz channel based on last channel tried and switch to DR_SF8C. Otherwise, the last attempt was DR_SF8C, so go back and try another 125kHz channel.

@frankleonrose
Copy link
Contributor Author

@jcwren Not sure what to suggest regarding the channel selection you're seeing. IIRC the subband selection is 0 based, so selecting the second sub-band group requires selectBand(1).

@jcwren
Copy link

jcwren commented Apr 21, 2017

If that's the case then I would argue that the comments are misleading and do not indicate that the intent is indeed to flip-flop between DR_SF8C and not-DR_SF8C when attempting to join.

That being said, why would you ever want to use any channel outside the enabled sub-band(s), regardless of whether you're joining or not? Certainly you can limit the sub-band once you've joined, but if you're operating on a network that you already know is in a certain sub-band, why waste battery power, time, and bandwidth on channels that won't exist?

If you agree that it should not, I can provide debug output showing the frequencies it's stepping through as it joins.

@frankleonrose
Copy link
Contributor Author

Clearly obeying the active channel map is the right thing to do. That's why Terry made this change in the first place, because the prior code was ignoring the map.

I'm not suggesting that the channel selection you observe is a good thing, just that I don't see how this code is responsible.

I recommend printing the active channels from the map (just 8 hex bytes) as well as the channel selected right after the do {} while loop.

@frankleonrose
Copy link
Contributor Author

@jcwren You could also try using The Things New York's fork https://github.com/things-nyc/arduino-lmic/commits/master

@terrillmoore
Copy link

terrillmoore commented Apr 21, 2017

@jcwren If you are seeing weird channel usage during join, please look at issue mcci-catena#1 and the related fixes. There was a definite bug (not at this location in the code) related to the reduction of DR -- totally US915-specific. The code generates bogus indices for an array and therefore does some odd things (and eventually ASSERTs, at least on my platform; but what it does is very much platform dependent, because it is using out-of-range indices on an array). I have noticed that the join behavior improved noticeably with all the fixes I made in my fork, but I have not had time (and don't expect to have time) to identify which fixes improved matters.

I have done substantial testing recently, and in my use (Cortex M0 and RISC-V CPUs, all with Hope-RF radios), the channel access pattern is exactly as expected. No use of channels outside the subband either pre- or post-join.

The related fixes have been pushed to The Things NYC, but I don't know if Frank has had time to review it.

@jcwren
Copy link

jcwren commented Apr 21, 2017

Gotta love github, so many forks and branches of code to choose from. Looks like I should switch to at least the The Things NYC branch, if not @terrillmoore's. Is there a compelling reason to pick one over the other?

@frankleonrose
Copy link
Contributor Author

My intent with the TTNNYC fork was to hew closely to Matthias' "original" and push our fixes to him. But since he hasn't been pulling, we diverge.

@terrillmoore has been producing a commercial product and has spent the most time both building and running hardware and debugging the code. He's less interested in minimizing change, so he may have some extra dependencies or things to support his C++ wrapper.

Starting afresh, I'd go with the mcci-catena to benefit from Terry's time.

@terrillmoore
Copy link

terrillmoore commented Apr 21, 2017 via email

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

4 participants