search

matthijskooijman / arduino-lmic

:warning: This library is deprecated, see the README for alternatives.
705 stars 651 forks source link

Should line 762 in lmic.cpp be right-shifting by 4, not dividing by 4? #8

Closed ReliableNetResults closed 8 years ago

ReliableNetResults commented 8 years ago

Hi,

Thanks for all your great work on getting this library ported - much appreciated!

Whilst trying to disable some channels in the US (915 MHz) spectrum using the disableChannel function (at Line 760 - not 684), I noticed that line 762 has the channel to be disabled dividing by 4:

LMIC.channelMap[channel/4] &= ~(1<<(channel&0xF));

whereas in the _nextTx function (at Line 817 - US variant) in the 125kHz section, I noticed that it determines whether next channel to transmit on is free by right-shifting by 4:

if( (LMIC.channelMap[(chnl >> 4)] & (1<<(chnl & 0xF))) != 0 ) {

I figure right-shifting is correct, since the channel map for the US (915 MHz) spectrum appears to be split up into 4x 16 channel segments (- 32-bits wide mind you!) for the 125 kHz-wide channels, with a 5th segment only 16 bits wide for the 500 kHz-wide channels.

Honestly, I'm not sure why there needs to be 4x 16 channel segments using 32-bit wide addressing. Surely only 1x is necessary plus a bit extra for the extended channels?

But perhaps I'm missing something... (...wouldn't be the first time!)

Does all that make sense? Please let me know if I've misunderstood the code.

Thanks again for all your hard work!

Andy

matthijskooijman commented 8 years ago

From a quick glance, your reasoning seems sound, but I haven't actually looked at the code yet. So far, I've only been working with (and looking at) the EU868 code, so I'm not familiar with this particular code. You could try asking IBM, but I don't think they're very responsive about this sort of thing.

I'll try to look closer, but I won't have time anytime soon. Feel free to further investigate and test if the change you propose actually changes / fixes things. If so, a pull request would be welcome :-)

matthijskooijman commented 8 years ago

I just had a closer look at the code, it seems that channelMap is intended as a bitmap with a single "enabled" bit for every channel. It seems there are 72 fixed channels, plus 2 dynamic ones, so that needs 74 bits. No single variable can hold that, so an array of 16-bit variables is used to spread out all the bits. In this light, >> 4 does indeed seem like the right approach, since that divides by 16, to get the array index that stores the bit for a given channel.

Would you care to submit a pullrequest for the /4 to >>4 change?

Honestly, I'm not sure why there needs to be 4x 16 channel segments using 32-bit wide addressing. Surely only 1x is necessary plus a bit extra for the extended channels?

Not sure what you're saying here. Where does the 32-bit come from?

matthijskooijman commented 8 years ago

This was fixed by #26, which was just merged.