search

helium / sx1302_hal

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

Report fractional RSSI #23

Open JayKickliter opened 2 years ago

JayKickliter commented 2 years ago

We'd ideally comply with JSON and report the entire float, but this formatting code is error-prone, and floats need to be constrained. Otherwise, we risk blowing the stack or way over-allocating memory.

CC/FYI @ke6jjj

ke6jjj commented 2 years ago

Agreed; the manual buffer sizing and calculation is one of the scariest things I have seen, but this is very tightly scoped and looks right (increasing the size by two to account for the decimal point and the tenths digit).

ke6jjj commented 2 years ago

Oh, but it's being performed potentially twice. Oops. Maybe the size should be plus four?

JayKickliter commented 2 years ago

I have a branch somewhere (maybe lost to bitrot?) where I got rid of the formatting altogether and built up a JSON object using parson (already used in the packet forwarder for parsing), then conversion to text at the very end. I should revise that and try upstreaming it.

ke6jjj commented 2 years ago

I think that could be overkill. Certainly the right thing to do, but this code is so rarely ever changed. I think this patch (with the correct sizing) is fine.

JayKickliter commented 2 years ago

Oh, but it's being performed potentially twice. Oops. Maybe the size should be plus four? see! That's why I dislike this manual buffer bookkeeping. I didn't think about the decimal point.

For future reference, another linux-only option is memfd + fprintf calls.

JayKickliter commented 2 years ago

but this code is so rarely ever changed

yeah, it was overkill and made a helluva diff, which is probably why I abandonded it. Our originall rationale (more GPS time info) for using a fork is gone, so the best course of action would be to either abandon this fork altogether or apply minimal patches to the current upstream code.

If someone can get to that before me I would be very happy.

ke6jjj commented 2 years ago

Ok, size looks right to me now, and I like that you dropped the lsnr -- it didn't have any additional precision to give anyways. (with current hardware, at least).