802.15.4 HIL updates; nRF52840 IEEE 802.15.4 Driver Updates

[bradjc]

Pull Request Overview

This pull request is a pass over the nRF52840 15.4 driver. The changes are fairly well encapsulated in commits, but listed here:

• Implement the config and power callbacks with a deferred call.
• Do not enter RX mode on init. RX mode is entered on start(). Otherwise the radio is always on, even if nothing is using it.
• Change constants to defines in the HIL.
• Remove setting RX and TX addresses as these functions were not doing anything and this support does not exist in the hardware.
• Change the catch for a "too long" packet to just set the 8th bit of the length to 0. This avoids both dropping the packet and any buffer index errors.
• Pass the ACK transmission error to the callback instead of the CRC result. Also rename the CRC variable. Also we know the CRC passed if we sent an ACK, so no need to check in the second RX case.
• Return OFF error if try to TX when radio is off.
• Format comments

This required some changes to the constants in the HIL.

• Add PHR_SIZE and PHR_OFFSET.
• Remove MIN_MHR_SIZE as it is incorrect (should be 7, as the minimum PHR is 9, and the MFR is 2, so that leaves 7).
• Add comments.

Testing Strategy

Need to do this.

TODO or Help Wanted

n/a

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Formatting

• Ran make prepush.

[tyler-potyondy]

Since we are dropping the CRC, I do not think this should be called frame_len since the MFR is technically part of the frame.

edit: this linked to line 854 when it should be to 855 the line below
let frame_len = data_len - radio::MFR_SIZE;

[bradjc]

What do you recommend? That variable is there from before.

[tyler-potyondy]

Perhaps changing this to buffer_len? buffer_len too feels suboptimal.

This gets at the issue with needing this field to be passed up the stack to denote the active length since we are passing the entire unused buffer upwards (so that it can later be reset by framer). These issues all seem like they would be solved by using a subslice. Is there a reason that we did not originally do this? In my opinion it seems the radio driver itself should be the only entity setting/resetting the receive buffer it interacts with (not the framer or some other "higher" element).

[bradjc]

This gets at the issue with needing this field to be passed up the stack to denote the active length since we are passing the entire unused buffer upwards (so that it can later be reset by framer). These issues all seem like they would be solved by using a subslice. Is there a reason that we did not originally do this? In my opinion it seems the radio driver itself should be the only entity setting/resetting the receive buffer it interacts with (not the framer or some other "higher" element).

The first 15.4 code was probably written before subslice/leasablebuffer existed.

I agree this should use subslice. But, the stack needs to edit both the start and end of the buffer, and that is more complex than many use cases. It sounded like the networking WG was thinking about that.

I would like to see the stack be clarified, and then updated to subslice once we have a good story for reserving space at the start of the buffer.

[tyler-potyondy]

Suggested change

	// Not enough room for CRC
	// Provided buffer does not have adequate space.

[bradjc]

This seems less clear to me.

[tyler-potyondy]

Only mentioning the CRC here is inaccurate since it may be that there is not enough room for the PHR or that there is not enough room for the CRC. Perhaps updating then to:
// Not enough room for CRC and/or PHR.

[tyler-potyondy]

I'm not entirely sure what the correct name is for this, but once again, frame_len seems somewhat misleading here since the frame does not contain the CRC here (that is to be added by hardware). There may not be a better name for this.