Open ryedwards opened 2 years ago
it doesn't make much sense to basically have an entire can.c/can.h that is split into two with a #define.
Agreed
What are the thoughts of creating a canfd.c and canfd.h?
Should be OK - we're already doing some amount of per-device-target config in CMakeLists. But why do you need a separate canfd.h , is it entirely different from the "API" in can.h ? I haven't looked at your code, just wanting to keep things as simple as possible.
Of course our codebase will need to be reworked and reorganized as we go forward - it has grown from supporting just one type of F0 board, to F042+F072, multiple boards, then F407, now this G0 port, and soon possibly CAN-FD (pending https://github.com/linklayer/gs_usb_fd/issues/2 and https://lore.kernel.org/all/20220310142903.341658-1-mkl@pengutronix.de/ )
(btw, did you implement CANFD or just basic CAN ?)
If you're interested, I have a (fairly out of date) branch that implements multitarget support for cmake, pulling in the correct cmsis, HAL, linker file, hal config, startup file, etc and passes correct compiler options for each core type. It ended up getting a bit messy but it does work! I have implemented FDCAN peripheral support on the G4 processor (for CANable 2.0), but it just does standard CAN TX/RX right now until kernel stuff gets pushed along a bit further.
https://github.com/normaldotcom/candleLight_fw/tree/multitarget
So the one I wrote has full implementation of CANFD per the latest gs_usb module.
I have the candleLight code compiling with the candfd.h structure but do see a way to get it working in just can.h/.c (the file will grow considerably but it may end up being a cleaner implementation).
Here is a link to my current G0 code: https://github.com/ryedwards/BUDGETCANFD_G0
My code sticks with the CubeMX codebase tightly (all custom code is in the USER blocks) so it can be regenerated in CubeMX. I'll need to check but it may be easier to revert my code to support the F0/F4 vs. forward porting the candleLight codebase. You would be able to use the F0 CAN HAL going this route vs. manual register manipulation. It is also coded to support the STM32 IDE right out of the box. The missing part is the CMake building utilities.
I have the candleLight code compiling with the candfd.h structure but do see a way to get it working in just can.h/.c (the file will grow considerably but it may end up being a cleaner implementation).
I'm perfectly fine with compile-time selecting the proper .c file depending on target (in fact some F4 code is almost annoying enough to justify that already), I was more thinking of having (if possible) a common .h file, if it's reasonable as an abstraction layer. I'm not sure if it's still realistic as we add FD support, I haven't looked closely.
My code sticks with the CubeMX codebase tightly
Understandable, but I don't think it's a direction we want to take for this project. I've had issues collaborating on cube projects where stuff starts breaking when you have the slightest version mismatch of cubemx or libraries, etc.
You would be able to use the F0 CAN HAL going this route vs. manual register manipulation.
In my experience HAL is fairly ROM-hungry. We already use it in other parts (GPIO, RCC), that's perfectly fine, but I would need compelling arguments to switch existing parts to HAL. Some targets with 16k flash are nearly full already.
Perfect. I'm about complete with the porting using the candleLight codebase. I should be done this weekend and will upload to my fork in git.
So I put it all together and gave it some light testing. There is FDCAN support is baked in (using the new structs within the latest gs_usb module) but I still need to update the bit timing routines. I did end up making all of the changes within the can.c file. The error parsing function still needs work. I was using the error IRQ callback in my code so I need to think of a better way.
Ok. Let me know when it's "ready enough" for a code review / PR
Oh hey @normaldotcom , I completely missed your reply up there. Seems like that branch diverged quite a bit, but I might steal some cmake bits eventually.
I've pushed my latest changes and I feel it's ready for code review. I made a few minor but fundamental changes that I think made sense as I was adding functionality. The main one was to move the CAN_INTERFACE and CAN_CLOCK_SPEED into the config.h as the CAN clock is more HW design dependent than it is chipset based. Also, INTERFACE varies based on how many CHANNELS you decide to implement.
Also, per your above, (@marckleinebudde correct me if I'm wrong) but fdcan was first available in the 5.19 mainline kernel. This is now available on the latest Ubuntu release (22.10). I've been testing my source against Marc's code changes for a few months and it is working. I've also integrated the latest gs_usb.h. This code fully supports CANFD. I've done a little bit of stress testing but nothing formal or with any real vehicle data. If anyone wants to play around with the G0 based HW I have a few extra I could ship out for testing.
I also reviewed @normaldotcom cmake goodies. I think many of those would make sense given the number of flavors of STM32s folks are using since we design around what we can find in the marketplace at the moment.
@ryedwards, please separate your changes by topic:
config.h
struct gs_host_frame
otherwise review is a PITA. :smile_cat:
Don't remove the USER_ID
flash support, there's a dedicated PR https://github.com/candle-usb/candleLight_fw/pull/102 for that.
STM32G4 support would be a new PR, after we've added STM32G0 support.
and at some point, run uncrustify with our .cfg P )
@ryedwards, please separate your changes by topic:
- update .gitignote
- move stuff to
config.h
- update
struct gs_host_frame
- add CAN-FD
- add stm32g0 hal
- add infrastructure for new µC to Cmake
- add new board
otherwise review is a PITA. 😸
Don't remove the
USER_ID
flash support, there's a dedicated PR #102 for that.STM32G4 support would be a new PR, after we've added STM32G0 support.
Creating so much extra work for me! :) I'll have to rethink verifying drivers for you in the future.
No problem., though That does make sense.
@fenugrec
In my experience HAL is fairly ROM-hungry. We already use it in other parts (GPIO, RCC), that's perfectly fine, but I would need compelling arguments to switch existing parts to HAL. Some targets with 16k flash are nearly full already.
I was having issues with the code playing nice with the non-HAL F0/F4 code and the G0 CAN code (The can handle pointer was making for some messy code). I ported over the F0/F4 code to the HAL and checked the ROM usage. It went from 16020 to 16312. So an additional ~300 bytes. Not trivial but an improvement in cleanliness and clarity. RAM usage is unchanged.
So PR #126 is all ready for review. There's a bunch of comments in the PR and on the individual commits.
@ryedwards I checked out your repo, switched to the implement-CANFD
branch and can build, but it doesn't look like any of the targets are using the G0 HAL, and none of the target .elf files even mention FDCAN in a basic strings
search. I don't see any (obvious) changes in the 14 commits in that branch which create a G0 target...
How do I build for canable 2.0 to play around with what you've done?
Hey @akohlsmith,
please try the multichannel branch in my repo: https://github.com/marckleinebudde/candleLight_fw/tree/multichannel. There's wokring support for STM32G0B1.
Thanks @marckleinebudde ; is it the budgetcan_fw
target I want to build for canable2.0?
semi-but-not-releated: is there a fork of slcand
which supports FD? The official stance is that slcand will not support FD since gs_can can do it all by itself.
In this image I see a STM32G431 not a STM32G0B1. I think you need to add the HAL, USB drivers, liker file, etc... for STM32G4 first. Then you can add your board.
Hey everyone. I already have written a codebase for the STM32G0 chip that supports gs_usb and shares most of the USB code with candleLight. I've used additional features in my code such as FreeRTOS and other HAL items that make it impossible to fully merge back into candleLight.
I've reviewed what would need to be done and found that the only major issue is the can handler. Since the FDCAN HAL is significantly different than the bxCAN on the F0 and F4 it doesn't make much sense to basically have an entire can.c/can.h that is split into two with a #define.
What are the thoughts of creating a canfd.c and canfd.h? Since most of the chips that support FDCAN (G0, G4, H7, etc) all use the same HAL it will be portable to those. I'm wondering what the code paradigm is with having split functionality in a single code base.
I am currently working on getting the code working in a fork with the canfd.x design.