mkuyper / basicmac Goto Github PK

My understanding is a little weak here, but I noticed that every time I reset the LMIC stack on AU915 (fixed bandplan) I get the same frequency for the first packet. That led me down a rabbit-hole.

Some questions:

1. This seems to be where the first packet is special-cased and the comment implies it's intentional. Do you know why?
2. The hoplist randomisation uses a PRNG algorithm, but ignores the existing source of RNG data (which can be hardware seeded). Changing the hoplist to use the getRndU1 (note: should rename this too) works and simplifies the code. Actually I could use getRndU2 there, since it seems to be constructing 16bits (in a 32 bit int? I need to re-read that to see why)
3. The DYN txChan also seem to start at the same place every time, so maybe there is a reason? Not sure if this is actually only on Join, but there's the odd place in BasicMac that sets the txChan to zero rather than a random one.

I will send a PR but want to confirm this is not intentional first, and maybe dig into the DYN case a little more.

Hey @mkuyper! You mentioned you're planning to make this repo the primary repo for BasicMac and would welcome pullrequest. As you've heard, we've done quite a bit of work at https://github.com/LacunaSpace/basicmac which would probably be good to merge with your work.

The work we've done is for a large part Arduino support (but this time while keeping the existing build system intact), improved portability (i.e. 8-bit AVR support), some actual fixes and a bunch of cleanups (in preparation of adding LoRa-E support, which is not yet public). I see that your changes mostly focus on the surrounding machinery (STM32 hal, services, board support), which we've mostly left untouched. That makes me hopeful that merging would be possible without too much conflict resolution.

However, before I invest the time to prepare a pullrequest for this, I'd like to discuss how we should do this exactly.

Some thoughts:

• We've collected quite a bit (140-ish) commits already (there mostly small and well-commented for easy review, though). It might make sense to submit these in multiple pullrequests separately (grouped by theme), but I expect that will make this a lot more work on our side. Also, reordering commits is probably going to be painful (even if there's no logical dependency, there's often textual dependencies between commits that make this tricky). So it would be my preference to do one bulk merge now, and then for future changes take a more gradual approach.
• I'm wondering if we should be doing an actual merge (so creating a merge commit and solving conflicts in there), or do a rebase of our changes on top of yours (fixing any conflicts in the original commits). The latter might be easier and produce a cleaner history, but the former ensures that the result is actually a descendant of our current, published, master branch, which prevents issues for people already pulling from our repo.
• I just quickly tried a merge, with gives a dozen or so conflicts, but nothing that looks hard to fix.
• If we're doing an actual merge, that means you'll get all of our commits verbatim. If there's anything in there that you wouldn't want to merge, we can't edit the original commits, but we'll have to revert or modify this in new additional commits (which is fine, I think).

So:

• Would you think a single-merge-with-conflicts-fixed is a reasonable approach?
• If you look through our the commit log, do you see any changes that you would object to at first glance?
• If you're happy with this approach, I'll do the merge and prepare a PR, we can continue more in-depth discussion in there.

In the Lacuna version / Arduino HAL, we're running into some hal timer-related problem that I'd like to pick your brain on.

The problem is caused because we didn't implement the 64-bit hal_getxticks(), because we don't have an easily accessible 64-bit counter (and I was a bit lazy). We could just implement it by extending the 32-bit timer we have (which we already do a bit to extends 32-bit micros to 32-bit 16-us-ticks), but I've seen that this xticks handling actually adds complexity, probably code size and IIRC wasn't implemented perfectly everywhere (i.e. truncating back to 32-bits in some places, voiding the advantages of using a 64-bit timer elsewhere).

So, I'm pondering to just remove the hal_xtimer() / osxtime_t (nearly) completely instead and just simplify things. The main question is, when is it actually used / needed? I haven't checked thoroughly before, but doing so now I find it is used for:

• Dutycycle available times.

  For these, 2³¹ ticks x 16μs = 9.5 hours should be sufficient (at 0.1% DC, this allows up to 34s of airtime for a single packet, which is over 500 bytes at SF12BW125 IIUC). Even more, the current code uses osxtime_t in updateTx and nextTx, but truncates to ostime_t in the nextTx return value anyway, so if the avail time would be > 9.5h in the future, I expect things to break with the current code already. Also, the actual available times are stored as 16-bit seconds anyway (from a base osxtime_t), so we could keep that and with some special care support up to 2¹⁶ seconds (18 hours) of (un)avail time as well (but maybe it's not even needed).

  One advantage of storing the base timestamp as osxtime_t currently, is that the availability times will remain valid even when there is no activity for > 9.5 hours (i.e. no activitity that causes setAvail to update the base timestamp), but this can probably also be handled by just updating the base timestamp once every 2³¹ or so ticks (maybe using a dedicated "overflow" task?).

• Tracking the wall time using MCMD_TIME_ANS / LMIC.gpsEpochOff.

  This code uses the wall time from the MCMD_TIME_ANS returned by the network to store an offset, which allows translating the result of hal_xticks() to wall time later. This can still work with a 32-bit ticks, but then the translation is only valid for 19 hours, after which the calculated wall time loops back. This can be solved by using a longer external timer, or maybe by Given there is no documented API for this (other than direct LMIC.gpsEpochOff access), maybe we can come up with some alternative (such as a getWallTime(ostime_t) function or so that works for timestamps within 9.5 hours around the current time as long as you call it at least once every 9.5h, or maybe the offset can be updated automatically internally whenever the timer overflow, using the same overflow task suggested above)?

• "Extended" scheduled callbacks (i.e. callbacks after 9.5 hours or more). It seems these are not used by BasicMac itself, only maybe by user code.

  If we want to keep this (which I guess could make sense when you do all scheduling of tasks in your application using the BasicMac scheduler), then we can still implement this even without having to keep hal_xticks(). This would require changing the API slightly (i.e. by specifying a relative callback, rather than an absolute time, which should be acceptable since these callbacks are approximate anyway) and then the implementation can be slightly changed by just counting off multiples of 2³¹ ticks instead of comparing against hal_xticks() on every intermediate callback (if implemented properly, this should be doable without stacking timing errors, so total accuracy is similar to the current implementation).

From your experience with the code, does the above seem plausible? Or am I missing something?

Summarizing, I think the availability time tracking can be internally improved to not require 64-bit timestamps without any user-visible changes, so I'll probably have a go at that.

Then, it might be useful for BasicMac to still offer an 64-bit timer, but maybe it can be made optional (and then the Arduino core could just not provide it, or we could add it anyway later, but people can disable it to save code space). If the 64-bit timer is enabled, the GPS offset and an absolute "extended" callbacks can be offered as is now. If the 64-bit timer is omitted, either these can just be omitted entirely (initially), or have limited / relative implementations as suggested above (implemented later maybe).

This was previously discussed as an aside in #10, but it deserves its own issue.

I previously wrote:

Then, for something completely different but related: I noticed ostime_t is signed, but in C signed overflow is undefined, so I think that might cause compilers to misoptimize the current code in some circumstance. I already spent some time converting ostime_t to unsigned (and adding the needed casts in places where a subtraction result must be interpreted as signed again), I'll share that patch later.

And:

I've made the timestamp types unsigned in LacunaSpace/basicmac@a6ccbfb as suggested above. Seems to have no significant impact on the (size of) generated code, but should be safer (though I heard someone recently argue that the "signed overflow is undefined" is not really mandated by the C spec, it's just gcc's interpretation of it, but since we're using gcc, I guess that's what we have to deal with). This changes is pretty broad, but I think it's good.

Then @mkuyper wrote:

ostime_t signed vs. unsigned arithmetic
Yeah. I've been aware of this for a long time. Technically, signed integer overflow is UB in C. Fortunately, we can pretty much count on being on a platform that uses two's complement and that doesn't raise an exception on overflow. The other, more tricky bit is that overflow being UB means that the compiler can pretend that overflow can't occur, and optimize accordingly. The way we are using ostime_t arithmetic pretty much precludes any of these optimizations. But you definitely need to keep your fingers crossed -- or enable -fwrapv on GCC.

Changing ostime_t to signed
This is a tough one, as it is a major breaking change to the API. In principle, it feels like the right thing to do, but this will break pretty much any application code that uses the runtime and does time stamp comparisons. Some of those might generate warnings, if enabled, but not all. We might need to rename the type if we make such a fundamental change.

As a side note, while unsigned overflow is well defined in C, a cast from unsigned to signed, e.g. from ostime_t to ostimediff_t in your no-xtime branch, is implementation-defined behavior. Admittedly, that's not as bad as UB, and thanks to the ubiquity of two's complement hardware it does the right thing pretty much everywhere.

A while ago I started an effort to get rid of global state in LMiC. As part of that, I also created a clean version of the runtime without global state that also fixed the signed overflow issues and included macros for creating time deltas and doing time stamp comparisons. I'll dig that up and throw it onto GitHub, maybe that can serve as some inspiration.

And:

Here's my no-global runtime concept I mentioned earlier: https://github.com/mkuyper/rtic/blob/master/rtic.h
Note that ticks are unsigned and there are macros for comparisons making it easier to do right thing.