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

[Fix] Update rssi status #7

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mwls-sean
Copy link

Currently server reads in radio status packets but never actually sets the rssi on the _status object of the UAV class, so downstream consumers never see it. This PR calls the update_status method to ensure rssi gets set accordingly.

@CLAassistant
Copy link

CLAassistant commented Jul 28, 2024

CLA assistant check
All committers have signed the CLA.

@ntamas ntamas self-assigned this Sep 26, 2024
@ntamas
Copy link
Member

ntamas commented Sep 26, 2024

We are investigating this right now and to be honest I don't understand yet why this line is needed; update_rssi() takes the existing RSSI list from the status object itself and changes a single entry in it, then updates the timestamp. Calling update_status(rssi=rssi) would make a copy of the RSSI list and assign the copy to the status object again.

@mwls-sean
Copy link
Author

We are investigating this right now and to be honest I don't understand yet why this line is needed; update_rssi() takes the existing RSSI list from the status object itself and changes a single entry in it, then updates the timestamp. Calling update_status(rssi=rssi) would make a copy of the RSSI list and assign the copy to the status object again.

No where in the codebase is there a call to update_status with rssi being passed in. The only hint of rssi being set is in handle_message_radio_status, using update_rssi. Before though, that's where things ended, with RSSI never reaching a place to be passed on.

@ntamas
Copy link
Member

ntamas commented Sep 26, 2024

When a new UAVStatusInfo object is created, it is assigned an empty list in its rssi property, here. In update_rssi, a reference to that list is taken here, and then the list is updated in-place. There's no need to call update_status() with an explicit rssi property.

That's the theory at least. That being said, @isti115 was testing this today and he also said that this extra line is needed. We will be out on the airfield tomorrow so hopefully at the end of the day we'll be able to come to a conclusion. Stay tuned.

@isti115
Copy link
Member

isti115 commented Sep 30, 2024

Well, we didn't end up testing this at the airfield, but I realized that I have a bunch of packets captured from a lab session, so I decided to investigate the issue. Currently it seems like the values appear just fine when replaying the previously recorded packets using the lastest commit (5675f80a) from the main branch of the server.

(The previous testing mentioned by @ntamas was actually performed by @thomas-advantitge, I only remember that he didn't seem to get any values in Live until he applied the patch, but I don't know the exact steps taken.)

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.

4 participants