[RFC] at86rf2xx: implement radio HAL

[jia200x]

Contribution description

This (draft) PR implements the Radio HAL interface for at86rf2xx. This is the first SPI radio that runs the Radio HAL, so I had to adapt some stuff that I would like to discuss with the community (see Discussion).

This port is resilient against #11256. After many debugging hours, forcing the radio to PLL_ON before a transition from RX_ON to TX_ON seems to be the only way to prevent the bug described there, since there's a small gap which makes it impossible to detect if there are incoming frames even when:
a) The transceiver state is checked before request_set_trx_state
b) The function checks if there are pending interrupts for the radio
c) The SHR detection is disabled right before checking a) and b)

The port has been tested against other radio in stressful conditions and it seems to work. There are some pending stuff before it's mergeable:

• Basic Mode support
• Periph implementation
• Some cleanup
• Adapt to gnrc_netif's thread_flag mechanism

And of course, it would nice to resolve the discussion presented above.

Discussion

Contrary to netdev, it was decided that the ISR offloading (a.k.a Bottom Half processing) would be independent of the upper layer, since not all radios require ISR processing or some might require a slightly different mechanism (e.g devices with 2 radios).

For radios that require ISR offloading, we need a Bottom Half Processing interface than can indicate there are pending IRQs and be able to process the events. I see 2 patterns that we might follow:

1. Implement a simple indication/response pattern that indicates the BH to call a driver specific "task handler" when an interrupt is triggered. This patterns was chosen for this draft and for this case 2 functions are defined:
   • at86rf2xx_irq_handler: To be implemented by the network stack. This function is called from ISR context and indicates the BH to process the radio events. The argument of this function (e.g pointer to the HAL) is assigned during at86rf2xx_init.
   • at86rf2xx_task_handler: This function process the pending IRQs and calls the Radio HAL CB functions.
2. Implement several mechanisms to process ISR functions with different mechanisms (event_thread, thread flags, msg_t, etc). This would require to implement functions like at86rf2xx_init_evq or similar to setup the BH.

The first approach has the advantage that it can be easily adapted to any network stack specific mechanism, with the cost of a BH implementation for every driver - network stack combination.
The second approach simplifies simplifies bootstrap code of the drivers. We could eventually decouple the initialization of the drivers from the initialization of the network stacks. The price to pay in this case is the fact he implementation of the drivers would be slightly more complex, since each driver has to implement variants of every mechanism. Also, the drivers would be hardcoded to a specific set of BH mechanisms

Opinions on this?
If there's a better way to process the events, please report it :)

Testing procedure

Try tests/ieee802154_hal and examples/gnrc_networking.

I haven't configured the dependencies properly. Therefore it's required to manually include gnrc_netif_events and netdev_ieee802154_submac for examples/gnrc_networking

Issues/PRs references

Depends on #16533 and #16534

[miri64]

• Basic Mode support

IIRC this isn't even implemented in current master, so do we have to wait for this additional feature before merging?

[jia200x]

IIRC this isn't even implemented in current master, so do we have to wait for this additional feature before merging?

It is :) :#13798

[miri64]

Opinions on this?

Just from a complexity stand-point option two seems more reasonable. From what I understand, option 1 needs (#network stacks ⨉ #network device drivers) abstraction layers and option 2 needs only #network device drivers abstraction layers. I am confused though, that you pointed this out as the drawback of option 2, so I am not sure I am seeing the whole picture.

[miri64]

IIRC this isn't even implemented in current master, so do we have to wait for this additional feature before merging?

It is :) :#13798

then nvm :-)

[jia200x]

Just from a complexity stand-point option two seems more reasonable. From what I understand, option 1 needs (#network stacks ⨉ #network device drivers) abstraction layers and option 2 needs only #network device drivers abstraction layers. I am confused though, that you pointed this out as the drawback of option 2, so I am not sure I am seeing the whole picture.

Option 2 requires "device drivers ⨉ BH mechanisms". On one side, we are using 4 BH mechanisms (msg_t, event_thread, thread_flags, pure ISR), which is in order with the number of network stacks. On the other side, I think we should slowly try to unify the BH of the drivers to e.g thread_flags or event_thread. In order to be compliant with the IEEE 802.15.4 standard the radios need some kind of determinism, which is hard to achieve with e.g message queues.

[jia200x]

ping @miri64 @maribu @fjmolinas @benpicco regarding the ISR offloading discussion :)

[miri64]

If it is all the same regarding number of adapter modules, I think from experience it is best to suit the driver side (so that least porting effort is needed for a device driver implementation). There are usually more device drivers incoming than network stacks, so we should keep the workload low there.

[jia200x]

If it is all the same regarding number of adapter modules, I think from experience it is best to suit the driver side (so that least porting effort is needed for a device driver implementation). There are usually more device drivers incoming than network stacks, so we should keep the workload low there.

So, this means going with the 2. approach? :) (e.g at86rf2xx_init_evq)

[miri64]

So, this means going with the 2. approach? :) (e.g at86rf2xx_init_evq)

Yes. Maybe we can even simplify that approach for driver devs more by making the BH processor independent of the mechanism used (as I did e.g. for sock_async). Then only one adapter would be needed. Of course such a wrapper must be very very thin at such a low layer, but if we could make it, I think driver implementation could be a breeze at least at that end.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[benpicco]

Would be really great to have this as it would allow to use the radio HAL for SPI based radios in addition to memory mapped ones.

[benpicco]

This one needs a rebase