search

things-nyc / arduino-lmic

LoraWAN-in-C library, adapted to run under the Arduino environment
26 stars 11 forks source link

setNextChannel() called with end channel plus 1 #15

Closed terrillmoore closed 7 years ago

terrillmoore commented 7 years ago

Hi @frankleonrose -- nice work.

In merging back to my codebase, I noticed that setNextChannel() is called either with setNextChannel(0, 64, ...) or setNextChannel(64, 64+8, ...).

The second parameter is called end; this implies it's the last channel. But the calls are coded as "last channel plus 1". Shouldn't these be setNextChannel(0, 63, ...) and setNextChannel(64, 64+8-1, ...)?

It looks harmless, provided that the number of enabled channels is less than the count.

frankleonrose commented 7 years ago

It's a [start, end) range specification, so the calls are correct. How would you prefer that were annotated? @param comment?

terrillmoore commented 7 years ago

But… end of the channel range is 63, not 64 (for the 125kHz). And end of channel range for 500 kHz is 71, not 72.

Usually would think it was (base, length) – in which case length is number of channels, or (first, last) where last is last actual channel. As used ssems to be (first, last+1) which is a very unusual API.

--Terry

From: Frank Leon Rose [mailto:notifications@github.com] Sent: Monday, March 13, 2017 10:39 To: things-nyc/arduino-lmic arduino-lmic@noreply.github.com Cc: Terry Moore tmm@mcci.com; Author author@noreply.github.com Subject: Re: [things-nyc/arduino-lmic] setNextChannel() called with end channel plus 1 (#15)

It's a [start, end) range specification, so the calls are correct. How would you prefer that were annotated? @param comment?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/things-nyc/arduino-lmic/issues/15#issuecomment-286126575 , or mute the thread https://github.com/notifications/unsubscribe-auth/ASb_75J24mCeKmP1mVJXaem6EqEyx4hDks5rlVUPgaJpZM4MapTV .

frankleonrose commented 7 years ago

Range notation: [start, (inclusive) end) (exclusive)

terrillmoore commented 7 years ago

If that’s the intent, we can close the issue. No big deal, just easy to miss. It’s not a public API so it’s not terribly important.

--Terry

From: Frank Leon Rose [mailto:notifications@github.com] Sent: Monday, March 13, 2017 10:39 To: things-nyc/arduino-lmic arduino-lmic@noreply.github.com Cc: Terry Moore tmm@mcci.com; Author author@noreply.github.com Subject: Re: [things-nyc/arduino-lmic] setNextChannel() called with end channel plus 1 (#15)

It's a [start, end) range specification, so the calls are correct. How would you prefer that were annotated? @param comment?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/things-nyc/arduino-lmic/issues/15#issuecomment-286126575 , or mute the thread https://github.com/notifications/unsubscribe-auth/ASb_75J24mCeKmP1mVJXaem6EqEyx4hDks5rlVUPgaJpZM4MapTV .