[riot-notifications] [RIOT-OS/RIOT] cpu/native: Allow Access to Hardware SPI Bus on Linux (#11352)
notifications at github.com
Wed Apr 24 15:34:34 CEST 2019
@kaspar030 Is there there anything else I can or should do to push the PR forward?
The style issues should be resolved, and for the Murdock failure, I again had a look at the output, with this being my interpretation of it:
The test has only a problem with [this line](https://github.com/RIOT-OS/RIOT/blob/59b4dcffed6b8f32e6fba01e262e941d7413f7ca/drivers/adt7310/adt7310.c#L69) in adt7310.c using the gnu toolchain (first failure) or llvm (second failure):
#define ADT7310_EXPECTED_MANUF_ID (0b11000000)
which could easily be changed to using its hexadecimal representation: `(0xC0)`.
The problem here is [this array](https://github.com/RIOT-OS/RIOT/blob/59b4dcffed6b8f32e6fba01e262e941d7413f7ca/drivers/pcd8544/pcd8544.c#L136) used by `pcd8544_riot(...)` to draw a sample image on the display, which is passed to [`pcd_write_image(const pcd8544_t*, const char*)`](https://github.com/RIOT-OS/RIOT/blob/59b4dcffed6b8f32e6fba01e262e941d7413f7ca/drivers/include/pcd8544.h#L123) and eventually is [casted to `uint8_t` in the internal `_write(...)` function](https://github.com/RIOT-OS/RIOT/blob/59b4dcffed6b8f32e6fba01e262e941d7413f7ca/drivers/pcd8544/pcd8544.c#L217). So I assume that the `_riot` char array is intended to be used as an array of unsigned bytes anyway, but that would also implicate changes to the API of that driver, as the char type is used there as well. (The [character table](https://github.com/RIOT-OS/RIOT/blob/59b4dcffed6b8f32e6fba01e262e941d7413f7ca/drivers/pcd8544/pcd8544.c#L38) is already using `uint8_t`).
Again, these failures occur because the result of a macro expansion of e.g. [`SD_CARD_DUMMY_BYTE`](https://github.com/RIOT-OS/RIOT/blob/59b4dcffed6b8f32e6fba01e262e941d7413f7ca/drivers/sdcard_spi/include/sdcard_spi_internal.h#L150) or [`SD_INVALID_R1_RESPONSE`](https://github.com/RIOT-OS/RIOT/blob/59b4dcffed6b8f32e6fba01e262e941d7413f7ca/drivers/sdcard_spi/include/sdcard_spi_internal.h#L52) is passed to a function that expects a `char` parameter. For values bigger than 0x7F, this generates an error because of a potential overflow.
So I'd suggest:
- Changing the binary to a hexadecimal constant for `ADT7310_EXPECTED_MANUF_ID` (this should not cause any trouble at all)
- Changing the types of the parameters of the drivers in question from `char` to `uint8_t`, which should also conform the [Types chapter of the Coding Convetions](https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions#Types). If the code could be compiled without error for other platforms by now, I assume that they already use an unsigned char type and this should not change anything.
If these changes are also reasonable from your perspective, I think there are to ways to go to get the build running:
1. Adding those changes to this PR
2. Creating a new separate PR for each driver and when they can be and are merged, rebasing this PR on top of that.
For the sake of clarity, I'd assume the second option being the best, as it would also allow an individual review by a maintainer for each driver.
Would one of those ways be suitable?
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the notifications