Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report fractional RSSI #23

Open
wants to merge 1 commit into
base: helium/hotspot
Choose a base branch
from
Open

Conversation

JayKickliter
Copy link

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
Copy link

ke6jjj commented Sep 24, 2021

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
Copy link

ke6jjj commented Sep 24, 2021

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

@JayKickliter
Copy link
Author

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
Copy link

ke6jjj commented Sep 24, 2021

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
Copy link
Author

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
Copy link
Author

JayKickliter commented Sep 24, 2021

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
Copy link

ke6jjj commented Sep 24, 2021

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants