littlefs-project / littlefs

A little fail-safe filesystem designed for microcontrollers
BSD 3-Clause "New" or "Revised" License
4.96k stars 775 forks source link

Feature request to add a macro for calculating CRC #251

Open umanayana opened 4 years ago

umanayana commented 4 years ago

The lfs_crc() should have a macro so users can provide there own implementation for performance reason.

For example I have a 32 bit CRC accelerator that I can use to perform CRC calculation and thus it would be handy to have a macro to allow for this.

geky commented 4 years ago

Hi @umanayana, thanks for creating an issue.

One idea behind lfs_util.h is that you can override the file if you define LFS_CONFIG to reference a your own header file.

make CFLAGS+=-DLFS_CONFIG=myconfig.h

This way users can redefine any of the utility functions. Additionally, this lets you define your own copies of stdlib functions (memcmp, strlen, uint32_t, etc) if the standard headers aren't available on your system.

I'd be happy to take in a PR that macroizes lfs_crc, but I don't know if it's necessary.

I've considered renaming lfs_util.h to lfs_config.h to make this more clear, but that might create confusion with the lfs_config struct, which is different.

assoc commented 4 years ago

One solution is to make lfs_crc a weak function, so it will be possible to define it somewhere else.

umanayana commented 4 years ago

One solution is to make lfs_crc a weak function, so it will be possible to define it somewhere else.

I find weak functions are a poor approach to developing robust API.

  1. Weak function do not strongly define what you actually want to occur.
  2. From an IDE perspective having multiple function with the same name cause IDE to ask the user which function they want to jump to. I know this main problem with eclipse IDEs and I find IAR does the same too.
  3. weak function increase build time because both source are built and only at link time does it get done.
  4. Depending on the type of projects you work on or the team you work with. I've had cases where Jr developer completely remove the strong function and weak function is built into source, however this is a user error but it an error that could have been avoided by not having it.

All in all I believe it's better to strongly define what is occuring. There are some every good uses cases for weak function especially when it comes to vector table being setup. But in most cases you can always find a better way to define what is happening in the code. It's better to have compile time errors than to find run time errors.

One idea behind lfs_util.h is that you can override the file if you define LFS_CONFIG to reference a your own header file.

I saw the macro and it's a good idea but many times macroing out all those methods is not very advantageous and require more works for the person trying to implement a port. I think adding a configuration file is very useful but this not mean replacing the current lfs_util.h The config file can just be used to define an ALTERNATIVE macros which would either enable default CRC or have the user define them.

If I do go with the stack I can send a pull request with the changes for the alternative macro.

On a side note I also think the callback in the configuration structure shouldn't use function pointer to define the porting layer. This produces worse optimization at the assembly level. I only use function pointer when I know I need to constantly change things on the fly during run time. You can get better optimized code if you just define the prototypes in a header and have the user write the definition in a porting layer. This also reduces potential bugs in the code, as you don't need to worry about a function pointer accidently not being set or checked correctly.