socket_zep: properly implement the radio HAL#19213
Conversation
a5ecf3d to
ec6e318
Compare
ec6e318 to
0edc540
Compare
8c563a0 to
c6d4dc3
Compare
c6d4dc3 to
4958c7f
Compare
60f9cb5 to
f3f0b82
Compare
78857f1 to
b8b8b65
Compare
b8b8b65 to
0025c20
Compare
0025c20 to
e82e746
Compare
f7c5e3b to
1585c82
Compare
1585c82 to
0f96544
Compare
mguetschow
left a comment
There was a problem hiding this comment.
No thorough review yet from my side, but just a minor thing I noticed at a short glance.
0f96544 to
7c9a24a
Compare
Teufelchen1
left a comment
There was a problem hiding this comment.
Hi!
I fucked up. I can't give you an in-depth review until the end of the week, as I promised.
Here are some questions I already collected.
I will hand in a complete review, including testing on Monday - if not, I owe you a Tschunk 😁
| #define SOCKET_ZEP_V2_TYPE_HELLO (255) | ||
|
|
||
| /* simulate RSSI by calculating error function of LQI */ | ||
| static const uint8_t lqi_to_rssi[256] = { |
There was a problem hiding this comment.
Where does this Link Quality Indicator array come from?
There was a problem hiding this comment.
That's a good question! The vagueness suggests that I had to 'massage' the data a bit to get result that looked nice - but in reality the RSSI - LQI relationship doesn't appear to be such a gentle curve. I just did a quick search and fond this paper that has a RSSI - LQI graph that looks like this
There was a problem hiding this comment.
@mguetschow fixed some issue with the LQI/RSSI conversion in the nrf52840_radio. Maybe he has some idea how this came to be?
There was a problem hiding this comment.
I think Nordic has their own understanding of LQI/RSSI and just assumes them to have a linear relationship. IIRC, they will not report both values at the same time, but LQI for each packet and RSSI just upon explicit sampling. Not sure if this helps here.
There was a problem hiding this comment.
Isn't any calculation of lqi to rssi or rssi to lqi, like guessing a persons weight from their height or the other way around?
There was a problem hiding this comment.
Yes, it's mostly for aesthetics / that applications that work with RSSI have something to work with, but RSSI is a bad metric to begin with, it's just provided to make the simulation more realistic.
|
I tested your change locally. I even did an interop test between your PR and the (more or less) current master. Everything worked as expected. You could be more generous with your comments / debug messages, especially here on native, where we don't need to worry about every byte ;) Beside the open comments above, I have nothing to add to this review. I am not super confident in my review here, e.g. ZEP is poorly documented (not in RIOT but in general), limiting my understanding here. Given that it is "just" native and very unlikely to be security relevant, I would still approve, once all comments are resolved. |
|
Is there anything I can do to move this forward? |
|
Squashy squash squash please |
socket ZEP now tries to simulate airtime.
c1500f5 to
45c6d78
Compare
|
Thank you 😃 |
Contribution description
The radio HAL implementation of
socket_zepwas a bit off.IEEE802154_RADIO_INDICATION_RX_DONEmust only be sent after the ACK was sent (if the frame contained an ACK request)_socket_isr()instead of fumbling around withMSG_PEEK | MSG_TRUNCTesting procedure
Build and run
examples/gnrc_networkingwithUSE_ZEP=1Start the ZEP dispatcher with 80% transmission success probability:
socket_zepshould now behave more like a real radio.100 unicast pings, 20% packet loss
-> retransmissions reduce loss, introduce duplicates
100 multicast pings, 20% packet loss
-> broadcast frames are not re-transmitted, giving us expected 20% loss, unicast replies are
tests/gnrc_rplstill workstests/gcoap_fileserverstill worksIssues/PRs references
follow-up to #16932