search

moonglow / pcan_pro_x

:alien: XCAN PRO/PRO FD/FD USB2CAN firmware implementation for cheap STM32F4 hardware
Do What The F*ck You Want To Public License
240 stars 148 forks source link

Make status led blink when device is alive. #9

Closed tomaszduda23 closed 3 years ago

tomaszduda23 commented 3 years ago

When it crashes the LED freezes.

moonglow commented 3 years ago

If you want to use led events on early stage ( while no ticks fired ), better to fix pcanpro_timestamp.c function pcan_timestamp_init just add HAL_IncTick(); to end of function. Another thing, why you put pcan_led_set_mode( LED_STAT, LED_MODE_BLINK_SLOW, 0xFFFFFFFF ); in 3 places ? If you want use that led like visual watchdog you can set it once in main.c as example. Btw are you sure what always blinking led is good solution ? i think it can disturb users

tomaszduda23 commented 3 years ago

why you put pcan_led_set_mode( LED_STAT, LED_MODE_BLINK_SLOW, 0xFFFFFFFF ); in 3 places ?

It blinks slow when power on and fast when USB is connected. I wanted to keep your old functionality which indicated that USB is connected. Pro and pro FD use different source file so I had to put it twice.

Btw are you sure what always blinking led is good solution ? i think it can disturb users

You put several asserts in your code and there is no way of knowing if device crash. At the begging I wanted to start blinking LED when crash but current implementation seems to be simpler. I needed a way to know that it crashed. Using gdb it not that easy.

If you want to use led events on early stage ( while no ticks fired ), better to fix pcanpro_timestamp.c function pcan_timestamp_init just add HAL_IncTick(); to end of function.

I can do that if you willing to accept this patch. If you are going to reject it does not matter :)

moonglow commented 3 years ago

I think your idea is OK, but i think better to blink fast if no USB drivers loaded and slows down if driver loads OK it will less disturb user ( because fast blink used for CAN data activity ), also need to remove pcan_led_set_mode( LED_STAT, LED_MODE_BLINK_SLOW, 0xFFFFFFFF ); from pcan_led_init and put it as example to main function or something like that, because led driver is not changed and make changes to it only for visual effect it is not good idea i think.

tomaszduda23 commented 3 years ago

Review was updated. I also added some doc.

moonglow commented 3 years ago

@tomaszduda23 Please do not update README.md file, i think such information can be located in project WIKI, I also do not understand why you names assert function like "crash" ... assert not equal to crash and can't be associated with crash. All other is OK and i will test is soon and merge. Thank you.

tomaszduda23 commented 3 years ago

I just removed README.md. I may open another PR for doc.

i think such information can be located in project WIKI

Is there one? If no it is better to put it somewhere and then just move when wiki is created. Otherwise I will forget to do so.

I also do not understand why you names assert function like "crash"

End user just want to know that device does not work correctly. It does not matter if there was assert, crash or shortage of RST. Anyway I can change the description.

moonglow commented 3 years ago

@tomaszduda23 Thank you for your contribution !

Is there one? If no it is better to put it somewhere and then just move when wiki is created. Otherwise I will forget to do so.

I think it be more useful to put such information to it, as example: variants of CAN mapping pins, LEDs actions, troubleshooting and so on