On Wed, Apr 9, 2025 at 2:02 AM Lwazi Dube <lwa...@gmail.com> wrote:
> On Tue, 8 Apr 2025 at 13:32, Xiang Xiao <xiaoxiang781...@gmail.com> wrote:
>
> > On Wed, Apr 9, 2025 at 1:16 AM Lwazi Dube <lwa...@gmail.com> wrote:
> >
> > > On Tue, 8 Apr 2025 at 05:23, Sebastien Lorquet <sebast...@lorquet.fr>
> > > wrote:
> > >
> > > > Hello,
> > > >
> > > > Nathan proposal to have a kconfig, with a default to the historical
> > > > algorithm, is a good solution.
> > > >
> > >
> > > Kconfig will work if you mean this:
> > >
> > > int16_t crc16(FAR const uint8_t *src, size_t len)
> > > {
> > > #if defined(CONFIG_CRC16_IBM)
> > > return crc16ibmpart(src, len, 0);
> > > #elif defined(CONFIG_CRC16_XMODEM)
> > > return crc16xmodempart(src, len, 0);
> > > #endif
> > > }
> > >
> >
> > It's a very bad idea to use Kconfig to change the function's behaviour
> like
> > this.
> >
> For example, y/zmodem calls crc16 which expects the result same as
> > crc16xmodempart.
> > With this change, y/zmodem works fine when you select
> CONFIG_CRC16_XMODEM,
> > but stops working when you select CONFIG_CRC16_IBM.
> > To make y/zmodem working with any choice, you have to change crc16 to
> > crc16xmodempart.
> > All code which calls crc16 before, must change to either crc16ibmpart or
> > crc16xmodempart, which means crc16 can't be used in any portable code.
> > Do you really want this behaviour?
> >
> No. Going forward, you should call crc16 if you want IBM. Otherwise call
> the right thing.
>
Kconfig was for preserving historical nuttx behaviour for those who don't
> want to keep up with upstream. It is not perfect.
>
So, we should change onewire_crc16 to crc16 per your suggestion:
https://github.com/apache/nuttx/blob/master/drivers/1wire/1wire_crc.c#L73-L98
When a user upgraded to this version with CONFIG_CRC16_XMODEM, 1wire driver
stopped working suddenly.
Changing the function behaviour by Kconfig doesn't improve any
compatibility, but makes the user write the portable code very hard.
