nrf52840 802.15.4 LQI #3972
nrf52840 802.15.4 LQI #3972
Conversation
Hardcode lqi value to be 0.
| @@ -134,11 +134,19 @@ pub trait RxClient { | |||
| /// - `header`: 8000 A fully-parsed representation of the MAC header, with the | |||
| /// caveat that the auxiliary security header is still included if the frame | |||
| /// was previously secured. | |||
| /// - `lqi`: The link quality indicator of the received frame. | |||
There was a problem hiding this comment.
Is LQI actually the correct term here, or should it be RSSI?
https://microchip.my.site.com/s/article/RSSI-vs-LQI-What-is-the-difference
There was a problem hiding this comment.
Interestingly, both rf233 and nrf52840 are calling this value "LQI" in their spec even though RSSI seems more accurate.
In the nordic case, the "LQI" is really just the median RSSI of the 3 RSSI values taken during the reception.
There was a problem hiding this comment.
I guess I'd fall on the side of going with what hardware says then, and stick with LQI.
| .schedule_upcall(upcall::FRAME_RECEIVED, (pans, dst_addr, src_addr)) | ||
| .schedule_upcall(upcall::FRAME_RECEIVED, (lqi as usize, dst_addr, src_addr)) |
There was a problem hiding this comment.
I know we didn't promise any stability here, but I'm still concerned about churn. Do you know why pan IDs and src/dst addresses where included in this upcall in the first place? They are in the packet, no? And it's not even possible to fit a 64 bit address in a 32 bit usize.
LQI is not in the packet itself, and that to me makes sense to include in the upcall arguments. I think I would rather see a new upcall be added (eg FRAME_RECEIVED_LQI or something) that only provides LQI. This is assuming there is not a use case for including the PAN IDs and src/dst addresses.
I think it might make sense to include other metadata in the future (for example, CRC check status for use cases where even failed crc packets are provided to userspace, or a flag indicating encrypted or not). If we find out that there is no use case for pans/dst_addr/src_addr we can just entirely drop that upcall in the future.
There was a problem hiding this comment.
As far as I can tell, the purpose of passing these values is to make the app's life easier. The header is passed with the packet so the app could recover this information. To my knowledge, there is not an explicit use for including these fields (beyond the ease of having these fields/avoiding recomputing these values).
I don't think we want to entirely drop this upcall since the primary purpose of this upcall is notifying the app that a packet is in the buffer. LQI may be needed by the app and the app has no way to recover this (as you mentioned and justifying LQI's inclusion). I'm in favor of removing pan and src/dest address for now and leaving only LQI in the upcall (with the potential for adding more later if the need arises).
-this logic is deadcode and no longer used by the capsule.
| // Try to decode the MAC header. Three possible results can occur: | ||
| // 1) The frame should be dropped and the buffer returned to the radio | ||
| // 2) The frame is unsecured. We immediately expose the frame to the | ||
| // user and queue the buffer for returning to the radio. | ||
| // 3) The frame needs to be unsecured. | ||
| let result = Header::decode(&buf[radio::PSDU_OFFSET..], false) | ||
| let result = Header::decode(&buf[radio::PSDU_OFFSET..(radio::PSDU_OFFSET+radio::MAX_FRAME_SIZE)], false) |
There was a problem hiding this comment.
Oh I meant to ask about these changes as well. These seem unrelated to LQI. Are they necessary?
There was a problem hiding this comment.
This gets at a major pain point in the 15.4 network stack...
The short answer is that the buffer given to the radio was increased by one byte (on nrf52840 and rf233 the lqi is placed by the radio after the packet).
Ideally, we would then only pass a buffer of the size of the radio packet (as done previously). However, we must pass the whole buffer since the buffer is given back to the radio after being passed up through the network stack (this is a major drawback of our current design). What this in reality means is that to keep a 130 byte buffer (size needed for psdu + packet + lqi), we have to pass a buffer of this size up the entire stack. Passing a slice results in the buffer "shrinking" each time a packet is received as the radio's buffer is then replaced with the now truncated slice. This is not a unique problem and is also caused friction with the PSDU offset PR a few weeks ago.
So the reason we have to now set the upper bound on the range is so that we don't pass the lqi as part of the packet to layers higher than the framer (since the buffer at the framer includes the lqi too). I'll go ahead and add a brief comment mentioning this.
-the packet must be sliced to not include the PSDU bytes or lqi byte before passing to layers above the framer. This is now explained.
Pull Request Overview
This pull request updates the 15.4 network stack to pass the LQI value up the stack. This updates the
receive()method to accept LQI as an argument. In both the nrf52840 and rf233 radios, the LQI value is found in the receive buffer following the last byte of data received. This implementation implements the LQI for the nrf52840 and not for the rf233 radio. For now, the rf233 LQI value is set to zero.This PR also updates the arguments passed to the userprocess. Previously, the pan, src address, and dest address were passed to the userprocess in the three available registers. The pan field has now been replaced by the lqi value as the pan field can be obtained in userland by re parsing the header.
Testing Strategy
This pull request was tested on the nrf52840.
TODO or Help Wanted
N/A
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.