search

skybrush-io / skybrush-server

Server component for Skybrush, an open-source drone light show and drone swarm management framework
https://skybrush.io
GNU General Public License v3.0
62 stars 32 forks source link

[Fix] Update rssi status #7

Open mwls-sean opened 2 months ago

mwls-sean commented 2 months ago

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 commented 2 months ago

CLA assistant check
All committers have signed the CLA.

ntamas commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 5 days ago

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