linklayer / cantact-fw

Firmware source files for the CANtact tool
http://cantact.io
Other
260 stars 179 forks source link

Fix USB enumeration-related compile signedness warnings #21

Closed brainstorm closed 3 years ago

brainstorm commented 3 years ago

Fixes compilation warnings of the sort (mentioned on https://github.com/linklayer/cantact/issues/3)... quick on-the-web type of patch/PR so tabs and spaces are garbled, but you get the point :-S

arm-none-eabi-gcc -DCORE_M0 -D HSI48_VALUE=48000000 -D HSE_VALUE=16000000 -D CANTACT_BUILD_NUMBER=0 -DSTM32F042x6 -IDrivers/CMSIS/Include -IDrivers/CMSIS/Device/ST/STM32F0xx/Include -IDrivers/STM32F0xx_HAL_Driver/Inc -IInc -IMiddlewares/ST/STM32_USB_Device_Library/Core/Inc -IMiddlewares/ST/STM32_USB_Device_Library/Class/CDC/Inc  -mcpu=cortex-m0 -mthumb -Wall -g -ffunction-sections -fdata-sections -Os -Os -c -o build/usbd_desc.o Src/usbd_desc.c
Src/usbd_desc.c: In function 'USBD_FS_ManufacturerStrDescriptor':
Src/usbd_desc.c:70:34: warning: pointer targets in passing argument 1 of 'USBD_GetString' differ in signedness [-Wpointer-sign]
   70 | #define USBD_MANUFACTURER_STRING "CANtact"
      |                                  ^~~~~~~~~
      |                                  |
      |                                  char *
Src/usbd_desc.c:227:19: note: in expansion of macro 'USBD_MANUFACTURER_STRING'
  227 |   USBD_GetString (USBD_MANUFACTURER_STRING, USBD_StrDesc, length);
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from Middlewares/ST/STM32_USB_Device_Library/Core/Inc/usbd_core.h:36,
                 from Src/usbd_desc.c:37:
Middlewares/ST/STM32_USB_Device_Library/Core/Inc/usbd_ctlreq.h:90:39: note: expected 'uint8_t *' {aka 'unsigned char *'} but argument is of type 'char *'
   90 | void USBD_GetString         (uint8_t *desc, uint8_t *unicode, uint16_t *len);
      |                              ~~~~~~~~~^~~~
trollcop commented 3 years ago

It would be nice if your changed did not introduce unnecessary whitespace modifications to the files. It's difficult to tell what was changed or not.

brainstorm commented 3 years ago

Agreed, it was a quick on-my-phone type of pullrequest, but this repo has not seen a merge since 2017, so closing.

It's unlikely it'll be acted upon, happy to be convinced otherwise by @ericevenchick or whoever maintains this repo... also, feel free to add my changes to your pullrequest in #20 or open another one yourself @trollcop ;)

normaldotcom commented 3 years ago

FYI I do have a fork of this repo with an updated HAL library with better performance on heavily loaded busses/etc. I haven't merged in hardware filtering yet, and I think I may have broken support for devices with external oscillators (if I remember correctly). I'm still doing active development on this fork.

https://github.com/normaldotcom/cantact-fw

brainstorm commented 3 years ago

Oh, nice work! While you are here, I have this burning question, let's see if you can address it since I'm genuinely curious:

https://github.com/linklayer/cantact/issues/3#issuecomment-752041563

TL;DR: The author, @ericevenchick, claimed that SLCAN is "generally not good" and that I should use gs_usb and another firmware entirely (candleLight) instead... what's your take on that? Is there really a perf difference and do you reckon you've corrected it in your fork?

Cheers and kudos for your work @normaldotcom!

normaldotcom commented 3 years ago

I would agree... slcan is definitely not the best option if you're running on Linux. Using the gs_usb / candlelight firmware gives better performance at high bitrates/busloads and eliminates the overhead of slcand. And it's just nice to plug it in and "can0" shows up immediately. I always recommend gs_usb when running on Linux with a reasonably modern kernel, the fact that you don't need to run slcand is enough motivation for me!