dronecan / DSDL

DSDL Protocol Description files for DroneCAN
MIT License
27 stars 63 forks source link

Added link stats packet #33

Open David-OConnor opened 1 year ago

David-OConnor commented 1 year ago

Follow-up to RC packet chat. Currently based on ELRS. Open to changes to make it more general A/R.

David-OConnor commented 1 year ago

I'm going to use this packet format unless the community wants something different.

hamishwillee commented 1 year ago

@David-OConnor Thanks for linking me in for co-ordination with MAVLink https://github.com/mavlink/mavlink/pull/1920.

I hope to discuss this in the MAVLink call tonight (I personally don't know much about radio links). I've reproduced the two proposed messages below.

I think they are relatively similar except that the MAVLink version seems to report RSSI/SNR from both ends of the transmission. It is not clear to me if the radio can get the information from the other end of the communication. If it can, then having that information would be useful presumably.

From an alignment perspective it would be good to have a similar naming convention and ranges, so we can easily map. After the call I will hopefully be able to communicate in a useful way on this.

#
# This definition describes metadata and statistics for a RF control link.
# It provides information regarding a link's signal quality, as well as transmitter
# power, and antenna information. It accomodates single-radio, and dual-radio
# link architectures. It provides information for both uplink (control unit to airborne
# receiver), and downlink (airborne receiver to control unit) data.
#

uint8 STATUS_RSSI_VALID = 1 # RSSI field is valid

# Uplink - received signal strength, in decibel-milliwatts (RSSI dBm) for antennas 1 and 2 respectively. 
# Ranges from # -255 to 0. (An integer value of 100 means RSSI is -100dBm)
uint8 uplink_rssi_1
uint8 uplink_rssi_2

# Link quality, on a scale from 1 to 255. Is a proxy for packet loss. 0 means data is unavailable. 1 and 255 are mapped to 0% and 100% of packets received.
uint8 uplink_link_quality

# Signal-to-noise ratio.
int8 uplink_snr

# This is an implementation-specific enum. May represent the OTA protocol used,
# and transmission rate.
uint8 rf_mode

# This is an implementation-specific enum describing the current transmitter nominal
# power level.
uint8 uplink_tx_power

# Statistics for downlink data, ie telemetry. See their respective `uplink` fields for
# descriptions.
uint8 downlink_rssi
uint8 downlink_link_quality
int8 downlink_snr

# Active antenna on transmitter and receiver. Example: 1 for antenna 1, and 2 for antenna 2. 
# Not applicable for single-antenna links.
uint8 tx_active_antenna
uint8 rx_active_antenna

Mavlink "Proposal": RADIO_LINK_STATS

- uint8_t flags (including RADIO_LINK_STATS_FLAGS_RSSI_DBM)
- uint8_t rx_LQ : (%) Link quality
- uint8_t rx_rssi1 : Rssi of antenna1
- int8_t rx_snr1 :  Noise on antenna1. Radio dependent. 
- uint8_t  rx_rssi2 : Rssi of antenna1 
- int8_t rx_snr2 :  Noise on antenna2. Radio dependent. 
- uint8_t rx_receive_antenna :  0: antenna1, 1: antenna2
- uint8_t rx_transmit_antenna :  0: antenna1, 1: antenna2
- uint8_t tx_LQ : (%)
- uint8_t tx_rssi1 : Rssi of antenna1. 
- int8_t tx_snr1 : Noise on antenna1. Radio dependent.
- uint8_t tx_rssi2: Rssi of antenna2
- int8_t tx_snr2 : Noise on antenna2. Radio dependent.
- tx_receive_antenna 0: antenna1, 1: antenna2
- uint8_t tx_transmit_antenna:  0: antenna1, 1: antenna2
David-OConnor commented 1 year ago

Added downlink LQ. Concur on all points. I prefer the uplink and downlink names since they're more explicit for things like SNR and LQ. Explicitly labeled RSSI as RSSI dBm. Using the names tx and rx for the active antenna fields.

olliw42 commented 1 year ago

on antenna, pl note that each unit does both receive and transmit and may handle antenna for both differently. Therefore you CANNOT just use one field like "active_antenna" but must use "active_antenna_used_for_receive" and "active_antenna_used_for_transmit". Since both sides of the link are represented in the message, there thus must be 2x2 = 4 active_antenna fields...

hamishwillee commented 1 year ago

I think we're now aligned in the sense that the data can be mapped between systems. We could be a bit closer if you chose to do this https://github.com/dronecan/DSDL/pull/33#pullrequestreview-1492242056 - but that would be up to you.

I'm not sure if we need more than this at this point or not.

David-OConnor commented 1 year ago

Given the DroneCAN RC channels packet was merged as 0-255, I'd like to keep that.

Thoughts on merging this? Thank you!

hamishwillee commented 1 year ago

Thoughts on merging this?

Thanks @David-OConnor .

From a MAVLink perspective "right now" this is fine - there isn't a 1:1 mapping but your current data could be mapped to our subset of the data by a flight stack. If we wanted to take your superset of fields we could add those too. I'm not planning on pushing the MAVLink side of this until someone wants to start using it, since I'm not going to implement it.

My concern here is that the number of people who have looked at this is quite small. Ollie, me, you. I worry that perhaps we just don't have a broad enough worldview. Worth spending a little more time to get more input?

I'll see if I can get more eyes on the MAVLink side of things. Are there more people you can involve in the DroneCan community?

tridge commented 1 year ago

@peterbarker can you have a look at this?

peterbarker commented 1 year ago

On the valid-data question, the rc-input packet uses a flags field to indicate which fields are present. Is there some reason not to use that pattern in here too, negating the question about "good invalid values"?

Should this be a per-antenna message, rather than having fields which are redundant in the non-diverse case? So include an antenna number rather than multiple (e.g.) RSSI fields.