search

helium / sx1302_hal

SX1302 Hardware Abstraction Layer and Tools (packet forwarder...)
Other
36 stars 44 forks source link

Fix timestamp normalization math, fixing rare timestamp bug. #14

Closed ke6jjj closed 3 years ago

ke6jjj commented 3 years ago

Occasionally the packet forwarder will forward a packet with an unparsable time timestamp attribute because the fractional part of the seconds field will have a negative sign in it.

An example that I witnessed was the timestamp:

2020-10-11T22:59:05.-00072Z

Bugs like this are almost always signs of buggy attempts to perform arithmetic on timeval structures in C without carefully covering all of the overflow and underflow conditions. Sure enough, that was the case here.

With this change, the code now properly handles a tv_nsec underflow and should no longer cause other parts of the code to generate unparsable timestamps like the given example.

JayKickliter commented 3 years ago

Thanks, and I'm sorry this PR fell through the cracks. I'm inclined to merge this as-is since we have a lot of faith in your code. The only complicating fact is that I want to rebase this fork (1) and the fewer changes the better.

1: even better, get rid of this fork altogether and use unmodified upstream code.

ke6jjj commented 3 years ago

No worries. I know it's thorny to deal with this when it would certainly be better to have the folks upstream merge it. To give you even more confidence: I've been running with this patch for about two weeks now. It certainly hasn't screwed up the normal case, but it's a little harder to prove it's fixed the bug.

JayKickliter commented 3 years ago

I've been running with this patch for about two weeks now

Good enough for me. Merging

ke6jjj commented 3 years ago

I also have the same fix running for the SX1301. Let me see if I can locate it and PR (if I haven't already). It will be in lora_gateway.

ke6jjj commented 3 years ago

Looks like yes, I already submitted the PR for that too.