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

AP_Logger: RXLQ in RSSI is in percent #26084

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

olliw42
Copy link
Contributor

@olliw42 olliw42 commented Jan 26, 2024

this is very tiny tidy up: Link quality (LQ) is pretty universally given in units of 0...100%.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with presenting it that way in the log, but the code that actually fills this packet needs to be updated to x100 the value before logging it (LogFile.cpp ~line 263)

@olliw42
Copy link
Contributor Author

olliw42 commented Jan 27, 2024

the code that actually fills this packet needs to be updated to x100 the value before logging it (LogFile.cpp ~line 263)

really? This would explain why the field RXLQ in the log_RSSI is float, but the code has then a bug:

your comment suggests that the intended range for LQ is 0.0 ... 1.0, but the code that fills the packet calls the function rssi->read_receiver_link_quality(), which in turn calls the function RC_Channels::get_receiver_link_quality(), which returns LQ as int16, and moreover the comment says it returns LQ as 0...100 %. RC_Channels::get_receiver_link_quality() in turn calls HAL functions, which are also int16.

!?!?

so, if RC_Channels::get_receiver_link_quality() is supposed to return a value in range 0...1, it only would return 0 or 1, and we would have a clear bug. Morever it's in contradiction to the comment.

ergo, the code as is I think is logging 0...100 in %.
If it was supposed to do 0..1 a conversion * 0.01f in rssi->read_receiver_link_quality() is missing, which would have to be undone as you say in the caller.

@peterbarker
Copy link
Contributor

Morever it's in contradiction to the comment

Thanks for delving. I'd prefer 0-1 myself, but it's probably the comment and float return value that needs fixing in the short term. This is all more complicated than it should be, too - I'm trying to move us towards removing RCInput from the HAL entirely, which would eliminate one layer!

@peterbarker peterbarker merged commit 381aba9 into ArduPilot:master Jan 31, 2024
92 checks passed
@peterbarker
Copy link
Contributor

Merged, thanks!

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