raiden00pl commented on PR #11428:
URL: https://github.com/apache/nuttx/pull/11428#issuecomment-2538193895
> We can move it to arch folder. It's used by both xtensa and RISC-V.
The point is that the entire RMT driver belongs to `arch` and should be
moved there.
> It isn't, it's used by either RISC-V and xtensa devices with the RMT
peripheral.
I can't agree here. It's vendor specific driver, only used by ESP chips, so
it should be in `arch`. A long time ago there was a similar case with `HRTIM`
which is a peripheral specific to STM32. It can be used to implement other
drivers like PWM, SMPS and so one. My initial implementation also included an
upper-half driver in `drivers/hrtim` which was eventually rejected by Greg,
because it's and stm32-specific driver. It doesn't matter if it is present in
stm32f3, stm32g4 or stm32h7, it is still a stm32-specific peripheral and of
course Greg's decision was justified and correct.
> We did this exactly the way you described for WS2812 upper-half. We didn't
implement it for RC, but we encourage community to do it (and we can help with
testing and validation). Then, I agree that we could even remove the RMT
upper-half driver (nuttx/drivers/rmt/rmtchar.c) because it isn't coupled with
the lower-half implementation.
RMT shouldn't be in `drivers/` in the first place, it belongs to `arch`
(yes, I know, I'm repeating myself :P) and IR feature should be provided with
already present driver that provides good and standardized abstraction:
`drivers/rc/lirc_dev.c`. Otherwise it looks to me like a violation of
`INVIOLABLES.md`:
```
### No Short Cuts
- Doing things the easy way instead of the correct way.
- Reducing effort at the expense of Quality, Portability, or
Consistency.
- Focus on the values of the organization, not the values of the Open
Source project. Need to support both.
- It takes work to support the Inviolables. There are no shortcuts.
```
> I agree that this is partially true for the RMT peripheral, but users can
move between different devices/architectures (from ESP32 to ESP32-C3, for
instance). About WS2812, this isn't true: the same application
(https://github.com/apache/nuttx-apps/tree/master/examples/ws2812, for
instance) can be used by any chip/arch/vendor that implements the driver.
I don't see a problem with WS2812, the problem is in RMT.
I wouldn't call the interchangeability of chips in a given vendor portfolio
good architecture. Good architecture is when I can replace chip from one vendor
to a chip **from other vendor**. Of course, assuming that the chips have the
same capabilities.
> In general, I agree with your considerations regarding the driver
organization, but I'm not convinced that we aren't providing that. Not allowing
binding a peripheral to an existing upper-half driver is different from
providing an upper-half driver for a peripheral whose capabilities are not
covered by the existing upper-half drivers. We implement our drivers thinking
that they could be bound to any other existing driver (just like the ws2812
upper-half driver)
There is no feature that RMT implements that cannot be implemented with
existing portable drivers. IR with `drivers/rc/lirc_dev.c`, and ws2812 as it's
implemented now, but with the difference that esp-specific ws2812
implementation use esp-specific API for RMT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
