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.
