drivers: at86rf2xx: Simultaneous use of multiple at86rf2xx device types #12914
Conversation
benpicco
added
Area: drivers
|
very nice one! |
drivers/at86rf2xx/at86rf2xx.c
Outdated
| #if IS_USED(MODULE_AT86RF212B) | ||
| for (int i = 0; i < num; i++) { | ||
| devs->base.dev_type = AT86RF2XX_DEV_TYPE_AT86RF212B; | ||
| devs->params = *params; | ||
| at86rf2xx_setup((at86rf2xx_t *)devs); | ||
| devs++; | ||
| params++; | ||
| } | ||
| #else | ||
| /* initialize device descriptor */ | ||
| dev->params = *params; | ||
| (void)devs; | ||
| (void)params; | ||
| (void)num; |
There was a problem hiding this comment.
Is it really needed to use the processor here? If no AT86RF212B is used, the function will never be called and garbage collected at link time. So it would not matter if this contains some actual code, when it is never called anyway.
Update: Fixed/clearified sentence above
There was a problem hiding this comment.
If I don´t wrap it with #if, "AT86RF2XX_DEV_TYPE_AT86RF212B" will not be defined.
The types are supposed to work as index for the arrays in at86rf2xx_properties.c.
It does not compile if I drop it.
E.g. someone has an AT86RF212B and an AT86RF233, AT86RF2XX_DEV_TYPE_AT86RF212B will be zero and AT86RF2XX_DEV_TYPE_AT86RF233 will be 1. If all types were defined, it would e.g. mess up something like at86rf2xx_rssi_base_values[dev->base.dev_type], if I don´t drop the #if´s in at86rf2xx_properties.c which would increase RAM.
There was a problem hiding this comment.
You could have them always defined, but give them some bogus value if the device is not used.
drivers/at86rf2xx/at86rf2xx.c
Outdated
| for (int i = 0; i < num; i++) { | ||
| devs->base.dev_type = AT86RF2XX_DEV_TYPE_AT86RF212B; | ||
| devs->params = *params; | ||
| at86rf2xx_setup((at86rf2xx_t *)devs); | ||
| devs++; | ||
| params++; | ||
| } |
There was a problem hiding this comment.
Just using devs[i].base.dev_type = AT86RF2XX_DEV_TYPE_AT86RF212B is more readable and with that you can drop the lines devs++ and params++. (The compiler will generate the same machine code anyway.)
Also: The cast from the more specific "object" to its parent "object" is not needed. You could just use at86rf2xx_setup(&devs[i].base) and have the type safety still in place. (Only when converting from the parent at86rf2xx_t to the more specific at86rf212b_t requires some magic. You can use the container_of() macro provided by kernel_defines.h for some limited type safety there: container_of() will only work if the more specific struct is indeed a "child object" of the at86rf2xx_t.
There was a problem hiding this comment.
Ok,
so I will use container_of() for any cast from the parent to the specific type.
Hmm, if I drop the caset (at86rf2xx_t*) in at86rf2xx_setup(&devs[i].base) my compile complains.
Because .base is of type at86rf2xx_base_t. The type at86rf2xx_t is at86rf2xx_base_t + at86rf2xx_params_t.
drivers/at86rf2xx/at86rf2xx.c
Outdated
| size_t at86rf2xx_get_size(const at86rf2xx_t *dev) | ||
| { | ||
| switch (dev->base.dev_type) { | ||
| default: return sizeof(at86rf2xx_t); |
There was a problem hiding this comment.
| default: return sizeof(at86rf2xx_t); | |
| default: |
This would allow the compiler to optimize this function much better if only a single type of AT86RF2xx devices is used: The default would just fall through to the single remaining case if only one device is used, so that all checks can be dropped in that case.
There was a problem hiding this comment.
we could actually drop this function if there are variant specific functions (see 98797e0#r356007674)
drivers/at86rf2xx/at86rf2xx.c
Outdated
| static void at86rf2xx_disable_clock_output(at86rf2xx_t *dev) | ||
| { | ||
| switch (dev->base.dev_type) { | ||
| default:; |
There was a problem hiding this comment.
| default:; | |
| default: |
drivers/at86rf2xx/at86rf2xx.c
Outdated
| uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__TRX_CTRL_0); | ||
| tmp &= ~(AT86RF2XX_TRX_CTRL_0_MASK__CLKM_CTRL); | ||
| tmp &= ~(AT86RF2XX_TRX_CTRL_0_MASK__CLKM_SHA_SEL); | ||
| tmp |= (AT86RF2XX_TRX_CTRL_0_CLKM_CTRL__OFF); | ||
| at86rf2xx_reg_write(dev, AT86RF2XX_REG__TRX_CTRL_0, tmp); |
There was a problem hiding this comment.
The variable uint8_t tmp needs a scope to live it. You could just enclose this all into a pair of curly braces, e.g.
{
uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__TRX_CTRL_0);
tmp &= ~(AT86RF2XX_TRX_CTRL_0_MASK__CLKM_CTRL);
tmp &= ~(AT86RF2XX_TRX_CTRL_0_MASK__CLKM_SHA_SEL);
tmp |= (AT86RF2XX_TRX_CTRL_0_CLKM_CTRL__OFF);
at86rf2xx_reg_write(dev, AT86RF2XX_REG__TRX_CTRL_0, tmp);
}or you can forward declare uint8_t tmp before the switch() statement
There was a problem hiding this comment.
I am gonna take the curly braces.
drivers/at86rf2xx/at86rf2xx.c
Outdated
| static void at86rf2xx_enable_smart_idle(at86rf2xx_t *dev) | ||
| { | ||
| switch (dev->base.dev_type) { | ||
| default: | ||
| (void)dev; | ||
| break; | ||
| #if IS_USED(MODULE_AT86RF233) | ||
| case AT86RF2XX_DEV_TYPE_AT86RF233: { | ||
| uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__TRX_RPC); | ||
| tmp |= (AT86RF2XX_TRX_RPC_MASK__RX_RPC_EN | | ||
| AT86RF2XX_TRX_RPC_MASK__PDT_RPC_EN | | ||
| AT86RF2XX_TRX_RPC_MASK__PLL_RPC_EN | | ||
| AT86RF2XX_TRX_RPC_MASK__XAH_TX_RPC_EN | | ||
| AT86RF2XX_TRX_RPC_MASK__IPAN_RPC_EN); | ||
| at86rf2xx_reg_write(dev, AT86RF2XX_REG__TRX_RPC, tmp); | ||
| at86rf2xx_set_rxsensitivity(dev, AT86RF233_RSSI_BASE_VAL); | ||
| break; | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
I think the RFA1 and the RFR2 also support smart idle and this was recently merged upstream.
There was a problem hiding this comment.
Ditto. With device specific functions (at86rf233_enable_smart_idle) we get rid of the switch-case.
There was a problem hiding this comment.
Here, a device specific function makes sens to me.
However, the calling function must check the device type
But the implementation of at86rf233_enable_smart_idle() and at86rfr2_enable_smart idle() would be the same.
So I think the best would be to take the core of at86rf2xx_enable_smart_idle(), keep the name, and make static inline at86rf233_enable_smart_idle() and at86rfr2_enable_smart idle()
static inline wrappers and the calling function (at86rf2xx_reset()) performs the switch case type checking.
There was a problem hiding this comment.
However, the calling function must check the device type
Why?
We could move that logic on top. I would assume the user of the raw driver knows which device is being used (AT86RF231, AT86RF233, etc). I would define the driver with variants specific prefixes (most of them can be just static inline wrappers to a common function) and let the layer on top (e.g netdev) device which function should be called.
Does this makes sense?
drivers/at86rf2xx/at86rf2xx.c
Outdated
| switch (dev->base.dev_type) { | ||
| #if IS_USED(MODULE_AT86RF212B) | ||
| case AT86RF2XX_DEV_TYPE_AT86RF212B: { | ||
| at86rf2xx_set_page(dev, AT86RF212B_DEFAULT_PAGE); | ||
| break; | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
How about adding a pseudomodule at86rf2xx_multidev that is automatically used in case more than a single type of the AT86RF2xx family is used. That would allow to optimize for the case only a single kind of device is used, e.g. like this:
switch (dev->base.dev_type) {
default:
#if IS_USED(MODULE_AT86RF2XX_MULTIDEV)
break;
#endif
#if IS_USED(MODULE_AT86RF212B)
case AT86RF2XX_DEV_TYPE_AT86RF212B: {
at86rf2xx_set_page(dev, AT86RF212B_DEFAULT_PAGE);
break;
}
#endif
}With the above code when only at86rf212b is used and no other type, the compiler knows for sure that it will just always have to call at86rf2xx_set_page() and no checks are needed.
There was a problem hiding this comment.
To me the issue here is that only Sub GHz radio have different page settings. Do we need a generic at86rf2xx_set_page?
As proposed in some other comments, I would define the common code for setting channel page and then only declare variants that make sense (at86rf231_set_page should not exist).
There was a problem hiding this comment.
we would get the same compiler optimization if we only use 2.4 ghz radios.
drivers/at86rf2xx/at86rf2xx.c
Outdated
| tmp &= ~(AT86RF2XX_TRX_CTRL_1_MASK__IRQ_MASK_MODE); | ||
| at86rf2xx_reg_write(dev, AT86RF2XX_REG__TRX_CTRL_1, tmp); | ||
| switch (dev->base.dev_type) { | ||
| default:; |
There was a problem hiding this comment.
| default:; | |
| default: |
drivers/at86rf2xx/at86rf2xx.c
Outdated
| default:; | ||
| uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__TRX_CTRL_1); | ||
| tmp &= ~(AT86RF2XX_TRX_CTRL_1_MASK__IRQ_MASK_MODE); | ||
| at86rf2xx_reg_write(dev, AT86RF2XX_REG__TRX_CTRL_1, tmp); |
There was a problem hiding this comment.
Again uint8_t tmp needs a scope to live in. You could use a forward declaration before the switch() or add this to its own scope by wrapping it into a pair of curly braces.
There was a problem hiding this comment.
Additionally, you code add pseudo modules at86rf2xx_spi and at86rf2xx_periph. The first would be always added if any of the SPI attached variants is used, and at86rf2xx_periph would be added if any of the peripheral variants (RFA1, RFR2) is used.
That would allow you to wrap the code above into:
#if IS_USED(MODULE_AT86RF2XX_SPI)
#endifAs a result, the ROM wouldn't increase if only the periph variants are used.
drivers/at86rf2xx/at86rf2xx.c
Outdated
| at86rf2xx_reg_write(dev, AT86RF2XX_REG__IRQ_MASK, | ||
| AT86RF2XX_IRQ_STATUS_MASK__TRX_END); | ||
| at86rf2xx_reg_write(dev, AT86RF2XX_REG__IRQ_MASK, en_irq_mask); | ||
|
|
There was a problem hiding this comment.
How about this instead:
switch (dev->base.dev_type) {
default:
#if IS_USED(MODULE_AT86RF2XX_SPI):
at86rf2xx_reg_read(dev, AT86RF2XX_REG__IRQ_STATUS);
#endif
#if IS_USED(MODULE_AT86RF2XX_PERIPH):
case AT86RF2XX_DEV_TYPE_AT86RFA1:
case AT86RF2XX_DEV_TYPE_AT86RFR2:
at86rf2xx_reg_write(dev, AT86RF2XX_REG__IRQ_MASK,
AT86RF2XX_IRQ_STATUS_MASK__TX_END | AT86RF2XX_IRQ_STATUS_MASK__RX_END;
#endif
drivers/at86rf2xx/at86rf2xx.c
Outdated
| #endif | ||
| } | ||
|
|
||
| void at86rfa1_setup(at86rfa1_t *dev) |
There was a problem hiding this comment.
I like this pattern of having variant specific functions. Does it make sense to do the same for most of the functions and implement at86rf2xx_ common only where it's needed?
|
As I was pulled into this somehow: A more descriptive PR title would be nice ;-). |
|
The file |
Ups... |
drivers/at86rf2xx/at86rf2xx_netdev.c
Outdated
| #if IS_USED(MODULE_AT86RFA1) | ||
| case AT86RF2XX_DEV_TYPE_AT86RFA1: { | ||
| irq_mask = ((at86rfa1_t *)dev)->irq_status; | ||
| ((at86rfa1_t *)dev)->irq_status = 0; | ||
| break; | ||
| } | ||
| #endif | ||
| #if IS_USED(MODULE_AT86RFR2) | ||
| case AT86RF2XX_DEV_TYPE_AT86RFR2: { | ||
| irq_mask = ((at86rfr2_t *)dev)->irq_status; | ||
| ((at86rfr2_t *)dev)->irq_status = 0; | ||
| break; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
I don't think it's useful to duplicate the code for those two.
You could also have a static inline _get_irq_status() function to get the #ifdef out of _isr()
There was a problem hiding this comment.
Ok, I can do a static inline static inline _get_irq_status().
Unfortunately, I cannot do:
#if IS_USED(MODULE_AT86RFA1) || IS_USED(MODULE_AT86RFR2)
case AT86RF2XX_DEV_TYPE_AT86RFA1:
case AT86RF2XX_DEV_TYPE_AT86RFR2: {
/* code */
}
#endifbecause the AT86RF2XX_DEV_TYPE_* is only known to the linker if the module is included.
But that´s part of the concept to reduce memory.
If I always have defined all device types, I always must have all values in at86rf2xx_properties.c defined to keep the index base property access working. And it would need more RAM. If the compiler sees that the program does the exact same thing for two cases, I thought it gets optimized to one case?
Do you prefer to have all types always available and for that increase RAM size?
There was a problem hiding this comment.
Hm I see. Of course I'd rather save on RAM ROM (those arrays are const so they'll reside in ROM).
If the compiler sees that the program does the exact same thing for two cases, I thought it gets optimized to one case?
I know the compiler does this for strings, but for code I wouldn't be so sure.
Anyway it doesn't matter in this case as you can't have both AT86RFA1 & AT86RFR2 enabled.
There was a problem hiding this comment.
On AVR they will end up consuming both RAM and ROM :-(
In this regard the von-Neumann architecture simply is better.
fabian18
changed the title
drivers/at86rf2xx/at86rf2xx_getset.c
Outdated
| switch (dev->base.dev_type) { | ||
| #if IS_USED(MODULE_AT86RF212B) | ||
| /* AT86RF212B has CCA_CS_THRES as bits [7; 4] in reg CCA_THRES, | ||
| so take care that we don´t override them. */ | ||
| case AT86RF2XX_DEV_TYPE_AT86RF212B: { | ||
| uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__CCA_THRES); | ||
| tmp &= ~AT86RF2XX_CCA_THRES_MASK__CCA_ED_THRES; | ||
| value = tmp |= value; | ||
| break; | ||
| } | ||
| #endif | ||
| #if IS_USED(MODULE_AT86RF231) | ||
| /* AT86RF231 also has CCA_CS_THRES */ | ||
| case AT86RF2XX_DEV_TYPE_AT86RF231: { | ||
| uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__CCA_THRES); | ||
| tmp &= ~AT86RF2XX_CCA_THRES_MASK__CCA_ED_THRES; | ||
| value = tmp |= value; | ||
| break; | ||
| } | ||
| #endif | ||
| #if IS_USED(MODULE_AT86RFA1) | ||
| /* AT86RFA1 also has CCA_CS_THRES */ | ||
| case AT86RF2XX_DEV_TYPE_AT86RFA1: { | ||
| uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__CCA_THRES); | ||
| tmp &= ~AT86RF2XX_CCA_THRES_MASK__CCA_ED_THRES; | ||
| value = tmp |= value; | ||
| break; | ||
| } | ||
| #endif | ||
| #if IS_USED(MODULE_AT86RFR2) | ||
| /* AT86RFR2 also has CCA_CS_THRES */ | ||
| case AT86RF2XX_DEV_TYPE_AT86RFR2: { | ||
| uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__CCA_THRES); | ||
| tmp &= ~AT86RF2XX_CCA_THRES_MASK__CCA_ED_THRES; | ||
| value = tmp |= value; | ||
| break; | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
The code is the same in all cases, so do you even need the switch?
There was a problem hiding this comment.
here is also a good candidate of static inline functions (e.g at86rf2_xxx`) that call the same function.
There was a problem hiding this comment.
E.g
/* at86rf2xx.c */
void at86rf2xx_set_cca_threshold(const at86rf2xx_t *dev, int8_t value)
{
uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__CCA_THRES);
tmp &= ~AT86RF2XX_CCA_THRES_MASK__CCA_ED_THRES;
value = tmp |= value;
}
/* at86rf231.h */
void at86rf231_set_cca_threshold(const at86rf2xx_t *dev, int8_t value)
{
at86rf2xx_set_cca_threshold(dev, value);
}
/* at86rf233.h */
void at86rf233_set_cca_threshold(const at86rf2xx_t *dev, int8_t value)
{
at86rf2xx_set_cca_threshold(dev, value);
}Etc
There was a problem hiding this comment.
All types have CCA_ED_THRES in CCA_THRESH register.
Inside the switch were types that also have CCA_CS_THRES.
But what is inside the cases would work for any kind.
So if it works for all kinds, a type specific static inline wrapper would not be necessary, right?
This should work for any.
void at86rf2xx_set_cca_threshold(const at86rf2xx_t *dev, int8_t value)
{
/* ensure the given value is negative, since a CCA threshold > 0 is
just impossible: thus, any positive value given is considered
to be the absolute value of the actually wanted threshold */
if (value > 0) {
value = -value;
}
/* transform the dBm value in the form
that will fit in the AT86RF2XX_REG__CCA_THRES register */
value -= at86rf2xx_rssi_base_values[dev->base.dev_type];
value >>= 1;
value &= AT86RF2XX_CCA_THRES_MASK__CCA_ED_THRES;
uint8_t tmp = at86rf2xx_reg_read(dev, AT86RF2XX_REG__CCA_THRES);
tmp &= ~AT86RF2XX_CCA_THRES_MASK__CCA_ED_THRES;
value = tmp | value;
value |= AT86RF2XX_CCA_THRES_MASK__RSVD_HI_NIBBLE; /* What is this? */
at86rf2xx_reg_write(dev, AT86RF2XX_REG__CCA_THRES, value);
}I could not find in any datasheet where
AT86RF2XX_CCA_THRES_MASK__RSVD_HI_NIBBLE
could come from.
There was a problem hiding this comment.
It makes sense to have this function as a common one.
There was a problem hiding this comment.
For this specific case, I think the conversion from dbm to internal values should be done outside of this function. Otherwise it removes all chances of caching the internal value and we would need to recalculate the internal value each time we call this function
kaspar030
changed the title
|
Providing separate In case of only a single variant of the AT86RF2xx is used, I guess this will have no impact on the ROM size, as modern compilers are pretty smart regarding inlining. When multiple variants are used, this will likely result in more ROM being used that a central function with a |
Same opinion here. I would have only one netdev_driver_t that calls the specific variant. Basically what I'm proposing is to avoid variant specific functionalities as generic AT86RF2XX functions. Most functions make sense as at86rf2xx (register access, set channel, etc). Some others only make sense for a giving variant or subset of radios (set page, smart idle, etc). So, who would call the specific variant function? The user of the driver (netdev, test, etc). |
drivers/include/at86rf2xx.h
Outdated
| } at86rf2xx_t; | ||
|
|
||
| #if IS_USED(MODULE_AT86RF212B) | ||
| typedef struct at86rf212b_params { |
There was a problem hiding this comment.
I think variant specific descriptors should be in their respective header file (e.g this one in the at86rf212b.h file)
There was a problem hiding this comment.
Schould e.g. at86212b_t and at86rf212b_params_t and at86rf212b_setup() be publicly available or just within the driver, e.g in at86rf2xx.c?
I decided to have them publicly available because if someone knows that he builds an application just for at86212b he could directly call at86rf212b_setup()
drivers/at86rf2xx/at86rf2xx.c
Outdated
| dev->base.state = AT86RF2XX_STATE_P_ON; | ||
| dev->base.pending_tx = 0; | ||
|
|
||
| switch (dev->base.dev_type) { |
There was a problem hiding this comment.
I think I would move this "dev_type" logic to an upper layer (e.g netdev). The user of the driver will always know which the variant is (e.g "at86rfa1_setup` function could work here)
drivers/at86rf2xx/at86rf2xx.c
Outdated
| { | ||
| #if IS_USED(MODULE_AT86RFR2) | ||
| dev->base.dev_type = AT86RF2XX_DEV_TYPE_AT86RFR2; | ||
| at86rf2xx_setup((at86rf2xx_t *)dev); |
There was a problem hiding this comment.
Here is an example of what I described above. You could call this line here and avoid the switch-case
drivers/at86rf2xx/at86rf2xx.c
Outdated
|
|
||
| void at86rfr2_setup(at86rfr2_t *dev) | ||
| { | ||
| #if IS_USED(MODULE_AT86RFR2) |
There was a problem hiding this comment.
as commented by @maribu somewhere, this IS_USED macro is probably not needed. If no one calls this function, it will get optimized out.
| IS_USED(MODULE_AT86RF232) || \ | ||
| IS_USED(MODULE_AT86RFA1) || \ | ||
| IS_USED(MODULE_AT86RFR2) | ||
| const int16_t _231_232_a1_r2_tx_pow_to_dbm[16] = { 3, 3, 2, 2, 1, 1, 0, |
There was a problem hiding this comment.
Since now all these variables can coexist, we can remove the IS_USED macros. They won't get to the binary file if they are not referenced by someone
drivers/include/at86rf2xx.h
Outdated
| * @param[in] params parameters | ||
| * @param[in] num number of transceivers | ||
| */ | ||
| void at86rf212b_setup(at86rf212b_t *devs, const at86rf212b_params_t *params, uint8_t num); |
There was a problem hiding this comment.
here and below, variant specific functions should be in their header file
fabian18
force-pushed
the
driver-at86rf2xx
branch
from
4315b7a
to
42f168c
Compare
|
@jia200x maybe I do not completely understand your concept, I am sorry.
switch (...)
#if IS_USED(MODULE_AT86RF231)
case AT86RF231: at86rf231_foo(); break;
#endif
#if IS_USED(MODULE_AT86RF232)
case AT86RF232: at86rf232_foo(); break;
#endif
#if IS_USED(MODULE_AT86RF233)
case AT86RF233: at86rf233_foo(); break;
#endifI feel like it would be better to keep type dispatching out of |
Yes, that's exactly the idea.
It's not written anywhere how netdev should be implemented, bit I also hope so :) One netdev should be enough.
This is a problem of the implementation of the _send and _init functions. In part because netdev is currently mixing transceiver and PHY layer stuff (see #12688 )
It's possible to have a mixture of at86rfxx and e.g at86rf231 to avoid having these kind of ugly ifdefs (I'm aware in most cases it's not needed to have duplicated static inline functions).
As described above, my concern is about adding pieces of a HAL here. I'm aware it's possible to optimize code with ifdefs but at the same time it makes it harder to maintain and test (and we try to compete with compiler optimizations). The driver is already quite hard to maintain and tests. I would propose it this way:
What do you think? |
|
I guess I will have to start over then because this is different from what I intended. |
| @@ -1530,6 +1530,7 @@ ISR(TRX24_RX_END_vect, ISR_BLOCK) | |||
| */ | |||
| ISR(TRX24_XAH_AMI_vect, ISR_BLOCK) | |||
| { | |||
| atmega_enter_isr(); | |||
There was a problem hiding this comment.
I don´t know where this went lost
|
@fabian18 thanks for the rework! I think the current concept matches what we discussed (individual devices,
For the Let me know if this is not clear.
What do you think? |
There is, in
The register addresses differ between SPI and MCU, but the MCU register address is the SPI register address plus some offset. But theoretically the offset could be different for any MCU. That´s why the functions in
There are different delays for awakening from sleep state and reset.
I think having e.g.
What / where is the presentation layer in RIOT?
If I wrote an application for one certain type I would exactly do that.
I could not figure out how to do that because the test should work for all different kinds. #if IS_USED(MODULE_AT86RF212B)
/**
* @brief AT86RF212B device array
*/
at86rf212b_t at86rf212b_devs[AT86RF212B_NUM_OF];
#endif
#if IS_USED(MODULE_AT86RF231)
/**
* @brief AT86RF231 device array
*/
at86rf231_t at86rf231_devs[AT86RF231_NUM_OF];
#endif
#if IS_USED(MODULE_AT86RF232)
/**
* @brief AT86RF232 device array
*/
at86rf232_t at86rf232_devs[AT86RF232_NUM_OF];
#endif
#if IS_USED(MODULE_AT86RF233)
/**
* @brief AT86RF233 device array
*/
at86rf233_t at86rf233_devs[AT86RF233_NUM_OF];
#endif
#if IS_USED(MODULE_AT86RFA1)
/**
* @brief AT86RFA1 device array
*/
at86rfa1_t at86rfa1_devs[AT86RFA1_NUM_OF];
#endif
#if IS_USED(MODULE_AT86RFR2)
/**
* @brief AT86RFR2 device array
*/
at86rfr2_t at86rfr2_devs[AT86RFR2_NUM_OF];
#endifand #if IS_USED(MODULE_AT86RF212B)
at86rf212b_setup(at86rf2xx_devs->named_devs.at86rf212b_devs,
at86rf212b_params, AT86RF212B_NUM_OF);
#endif
#if IS_USED(MODULE_AT86RF231)
at86rf231_setup(at86rf2xx_devs->named_devs.at86rf231_devs,
at86rf231_params, AT86RF231_NUM_OF);
#endif
#if IS_USED(MODULE_AT86RF232)
at86rf232_setup(at86rf2xx_devs->named_devs.at86rf232_devs,
at86rf232_params, AT86RF232_NUM_OF);
#endif
#if IS_USED(MODULE_AT86RF233)
at86rf233_setup(at86rf2xx_devs->named_devs.at86rf233_devs,
at86rf233_params, AT86RF233_NUM_OF);
#endif
#if IS_USED(MODULE_AT86RFA1)
at86rfa1_setup(at86rf2xx_devs->named_devs.at86rfa1_devs);
#endif
#if IS_USED(MODULE_AT86RFR2)
at86rfr2_setup(at86rf2xx_devs->named_devs.at86rfr2_devs);
#endifin each application, instead of wrapping this inside a struct or a function respectively? Most importantly I will try to keep a single |
The array approach should work. And yes, having one reg_read and reg_write functions simplifies a lot all the layers on top.
The FB information is indeed different, but accessing the framebuffer is an abstract operation. For getting ED level, IMO the netdev "read()" function should care about this. The "read framebuffer" function should be agnostic to the content.
The Presentation Layer is basically that translates from deep operations (SPI, MCU reg access) into some logic that can be reused by the driver. What I meant with this, is the fact all these reg functions can be written to be device independent, as you described above.
A user of a single radio should directly use the device descriptor (as discussed above).
An application that should work for all transceiver types should use an abstraction (e.g
Consider there are other patterns that have the same effect (array of pointers, linked list, etc). This moves the allocation logic to the device user and not to the device driver. |
boards/nucleo-f767zi/Makefile.dep
Outdated
| @@ -1,5 +1,5 @@ | |||
| ifneq (,$(filter netdev_default gnrc_netdev_default,$(USEMODULE))) | |||
| # USEMODULE += stm32_eth | |||
| USEMODULE += stm32_eth | |||
There was a problem hiding this comment.
I commented this out to test exampes/gnrc_networking, but with the ethernet driver it was hanging during SPI register access, although I set GNRC_NETIF_NUMOF := 2 (ethernet + at86rf233). But this is not the topic of this PR.
|
So far I am struggling to have a common |
In that case I think it's ok to have an
|
I am pretty much in favor of this! |
|
hi @fabian18 what was the reason to close this PR? This is a nice cleanup that would improve quite a lot the maintainability of these drivers! |
|
Hi @jia200x, My new design choice would be to have a module |
Contribution description
This pull request updates the
AT86RF2XXdriver while it adds functionality to manage more than justone type of
AT86RF2XXdevice at the time. Currently RIOT supports external SPI based transceivers:and MCU integrated trasceivers:
The implementation concept was to add a dev_type member to the at86rf2xx_t structure.
Then there exist the types
at86rf212b_t,at86rf231_t,at86rf232_t,at86rf233_t,at86rfa1_tand
at86rfr2_t. Depending on thedev_typemember anat86rf2xx_t*is assumed to be safely castableto the distinguished device. Explicit device types may implement additional members and they
have their correlating
_params_ttype because the number of connected devices is derived fromther
params. As well as device structures,paramsstructures could have additional members and somay vary in size.
The size of the driver scales with the number of transceivers, it was compiled for,
following the case differentiation pattern:
However this does not propagate to application code.
Testing procedure
For testing I used a
nucleo-f767ziwith anAT86RF233on the one side and amicroduino-corerfwithan
AT86RF233on the other side and tested the application intests/driver_at86rf2xxwithtxtsnd.Since I don´t have any other type by my hand, maybe others with more transceiver types could also
test.
Nucleo side:
Microduino side:
Issues/PRs references
#4876