Byte array should be uint8_t, not char

Development archive
1

Hi,

The Coding Convention is clear about it.

"Guidelines for pointer types (as long as it is reasonable):

  • use char * for strings and only for strings
  • use uint8_t[] as type for arbitrary byte buffers, but use void * to pass them around. uint8_t[] because we’re dealing with bytes and not characters, void * to avoid unnecessary casting shall the need arise to have their content to have a certain type
  • use uint8_t * to pass “typed” byte buffers, e.g., link-layer addresses, where it avoids unnecessary temporary variable
  • use void * for generic typing"

In the SPI driver however the transfer functions use char * parameters, but SPI is usually dealing with binary information (bytes), not strings. This leads to unnecessary casts in other parts of the code. (E.g. nvram_spi).

What is our policy about this? Are we going to correct this at some point? Is it too late already (I hope not)?

2

Hi Kees,

Unless there is a good reason to deviate from this guideline all violations should be corrected. This particular rule was added relatively recently, so it would not surprise me if not all occurrences in RIOT have been adapted yet.

Cheers, Ludwig

3

Hi Kees, Like Ludwig wrote, there are some places which haven’t followed the coding conventions because the CC weren’t as clear before as they are now with regards to this. Since RIOT is relying on its community for code contributions, these kinds of clean ups may take a long time after the documentation has been updated until someone decides to fix them.

The SPI periph driver is going through some rework, see https://github.com/RIOT-OS/RIOT/pull/4780 and https://github.com/RIOT-OS/RIOT/issues/4758, but it has not yet been merged because of various other things getting in the way.

Best regards, Joakimr

4

Thank you for the info. It seems that gebart noticed it too :slight_smile: There is a fresh reply in #4780.

5

Hi Ludwig,

Well, it will be a challenge to smootly correct this. There are 16 CPU's that use spi_transfer_byte(s) and 6 drivers.

I won't mind creating a PR, but of course I can only test it by building examples for all boards that support SPI. And look at compile errors. Or are there other procedures?

6

Hi,

gebart is Joakim :wink:

Cheers, Ludwig

7

Hi Kees,

I assume there are more violations than the ones in the SPI drivers... But of course every step forward is great!

Testing can be done by the community. We have plans to create a distributed test system with actual hardware attached, but sadly that has not become reality yet.

Compilation can be tested automatically by running `make buildtest` for the respective test application (tests/periph_spi).

Cheers, Ludwig

8

Hi Kees, Do you have the time/energy to resume the effort of updating the periph/spi interface in where Hauke left off in https://github.com/RIOT-OS/RIOT/pull/4780?

There are a lot of device drivers which need updating, and also some existing comments on the PR.

I can assist in reviewing the changes and updating device drivers, but I don’t have a lot of time to spend on this right now.

Best regards, Joakim

9

Hi Joakim,

I've done a PR for SPI. https://github.com/RIOT-OS/RIOT/pull/5901 If you have a chance, can you have a look at it/

What this set of changes shows is that several developers have been inserting casts to be able to get SPI code compile without errors. These casts (from uint8_t to char) are now not needed anymore.

Yes, there were quite a few files to be modified, but the changes were straightforward. I even patched a patch file :slight_smile: See pkg/u8g2/patches/0002-u8g2-add-riot-os-interface.patch -- Kees