-
Notifications
You must be signed in to change notification settings - Fork 292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for the STM32Gxx chipset and add CANFD functionality to the driver #126
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of work went into this, I can tell. Thanks for splitting the commits.
Some comments sprinkled through my review.
For struct gs_host_frame, do I understand correctly that it could be chosen at compile-time ? I.e. on a CANFD device, does the kernel driver always send the larger struct (even if the channel is opened in Classic mode) ?
(Trying to see if a simple typedef would do the trick)
Allocating the fifos with the large struct is indeed a waste - smaller targets can't afford the RAM. In fact, the CI tests fail at the linking step due to overflowing RAM. Will look at this later.
Would prefer not to reduce fifo depth.
src/main.c
Outdated
last_can_error_status = can_err; | ||
} else { | ||
queue_push_back(q_frame_pool, frame); | ||
// If there are frames to receive, don't report any error frames. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happened to these ~15 lines' whitespace ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. git had a hard time with the diff on this file. You have to turn your head sideways to line everything up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah... I'll try reverting the whitespace changes here (i.e. when I do the final merge), it's too distracting
I added the changes to support resizing the gs_host_frame for non-CANFD builds. |
There were a few major issues causing the F0/F4 to not operate on CAN. Simple things that I missed. Will commit the changes shortly. Is there anyone that can test the code on a few other devices? |
I lent almost all my prototypes, I just have one left. Can test enumeration etc but not much else atm |
I have an extra prototype of the budgetcan if you are interested in testing it out or just adding it to your spare electronics bin. |
src/can.c
Outdated
@@ -1,26 +1,26 @@ | |||
/* | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess turn off whitespace for review. I tried to "uncrustify" the file on my own and forgot that I turned off whitespace checking in my GitHub Desktop.
Well crap I tried to squash two commits I think I broke something. Let me know if this is a problem. |
Sure, how about a trade, I can spare one of mine (bonus : galvanically isolated). |
Fire me an email and we can exchange info (bonus : 2 FDCAN channels, 1 SWCAN channel and 1 LIN) :) |
src/can.c
Outdated
// The STM32F0 only has one CAN interface, define it as CAN1 as | ||
// well, so it doesn't need to be handled separately. | ||
#if !defined(CAN1) && defined(CAN) | ||
#define CAN1 CAN | ||
#endif | ||
|
||
// Completely reset the CAN pheriperal, including bus-state and error counters | ||
static void rcc_reset(CAN_TypeDef *instance) | ||
static void rcc_reset(CAN_TYPEDEF *instance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fenugrec I've found an issue with this function. For FDCAN multichannel devices if you reset the clock when you enable one channel it will kill the other channel. Since all channels in FDCAN use the same clock I noticed that the CubeMX generated init functions use a global counter to track when to enable/disable the clock (there is no handler for just resets). At this point I am going to remove this reset handler for the FDCAN implementation.
src/can.c
Outdated
#ifdef nCANSTBY_Pin | ||
HAL_GPIO_WritePin(nCANSTBY_Port, nCANSTBY_Pin, GPIO_PIN_SET); | ||
HAL_GPIO_WritePin(nCANSTBY_Port, nCANSTBY_Pin, nCANSTBY_Active_High == 1 ? GPIO_PIN_SET : GPIO_PIN_RESET); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also should have tested this area better before I opened the pull request but it was masked by the clock reset issue. This design will not work (each channel has it's own standby pin). Also I will need to redesign the termination resistor design. I'm thinking as this point I create another PR to handle those two as it will be a big change to implement it properly. I don't think "per channel" GPIO was ever considered in the design. For now the 2nd and 3rd channels will not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, let's focus on just single-channel canFD for now
So as of right now I've tested all of the functionality on my STM32G0B1xx hardware and on a canable (STM32F042xx). Sanity checks out. As discussed above the CANFD only works for a single channel right now but since I think I'm the only one with FDCAN capable hardware I'll live with it. |
btw you should really really be working on a branch, not directly on master... will make it easier to rebase as I slowly merge in the easier commits |
The Linux driver sends classic-CAN frames if the device is opened in classic-CAN mode. Currently it will always send CAN-FD sized frames if the device is opened in CAN-FD mode, even if sending classic-CAN frames. Although it might be an optimization on the Linux side to send classic-CAN frames in CAN-FD mode, if a classic-CAN frame is send.
ACK For completeness: On the receiving side under Linux, as soon as at least one channel (of a multi channel device) supports CAN-FD mode and/or timestamps, the RX USB frame size is setup accordingly. All CAN channels are multiplexed into the same endpoint, so we have to deal the maximum USB RX size possible. |
include/gs_usb.h
Outdated
@@ -275,34 +275,44 @@ struct gs_device_termination_state { | |||
u32 state; | |||
} __packed; | |||
|
|||
struct gs_host_frame { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to port the DECLARE_FLEX_ARRAY
from BSD to the candlelight FW, so we can make use of it here, too.
The way the firmware is written it can handle any size incoming. But, if the device supports CANFD the buffer sizes default to the CANFD frame size pre-compile. As for bus bandwidth, the firmware will send "classic can" frames (24 bytes) when it's a non-FD frame and the full "can" frame (80 bytes) when it's FD data. I think the kernel does the same.
I did create a pre-compile define for devices that don't support CANFD (276fb49). Those devices will only use classic can buffers. All of the devices that support CANFD have tons of RAM so there's not a real concern with those. |
Agreed. Apologies for the delays, part of the issues in merging this "as-is" is that some commits break the build - temporarily, of course, but I haven't decided how much it bothers me. I started reordering / rebasing commits in a separate branch, but that would make any further work on your side much more complicated. |
From my point of view breaking builds is not desirable (/me wearing my Linux Maintainer glasses). |
Can you push that branch somewhere? |
I took a look at it. It's going to be a huge tear-up to the GPIO design to assign ports/pins to different channels. I tried a few ways and the best bet was to create indexed lookup tables for all GPIO. The biggest issue right now is that the gpio.c tries to handle all of the GPIOs for all the different boards. It's becoming a mashup of defines that'll be even worse if I add channel based GPIO. |
IMHO this should be different topics:
Not necessarily needed to be addressed in this order. |
Then I suggest leaving multichannel well alone until G0 + CANFD gets merged, and then give me time to start splitting out some code into platform-specific folder/files. I think we have just about enough #ifdefs in can.c and gpio.c ...
Will do - possibly even later today. |
https://github.com/fenugrec/candleLight_fw/tree/g0_1 WIP of course, could be rebased / push -f'ed at any time . I was cherry-picking + rebasing a few commits at a time. |
That looks really nicely cleaned up! |
Just updated https://github.com/marckleinebudde/candleLight_fw/tree/multichannel The firmware is now free of dynamic memory management, everything is allocated statically. The queues are replaced by a Linux compatible list implementation. The last patch sets the heap size for the F042 and F072 to Next I'd continue to work on multi channel support....and I need a better name for the struct that holds the
|
At what point does it make sense to close this PR and open new ones to merge in the changes on various branches? I think my changes incorporated many of the features that @marckleinebudde is including, just not as elegantly :) Looks like the branch created by @fenugrec already has done 90% of this. I just need to help out with the CANFD part. Again, github newbie here. Thoughts? |
Also, to support multi-channel, does it make sense to move the CAN RX into an interrupt driven design vs. the current poling design? For multichannel polling you need to create a for loop to poll each channel in main. With the interrupt you simply add to the list within the interrupt (skip a whole extra step in the main loop). I've been running it in my budgetcan codebase for a while and it saves on ROM and makes it so that when messages come in they are just added to list immediately. Again, thoughts? |
@ghent360 I've updated my branch again. The LEDs are not properly integrated, yet. 2 Channels and CAN-FD are working. |
Merged and tested. The CAN_btconst.features were wrong again. Somehow the IS_ENABLED(CONFIG_CANFD) was not working correctly. After updating that. it is working again. Separate topic: I tested the canboot bootloader from the klipper folks, it is convenient since the mks UTC board does not have a good way to set 'boot0' and enable DFU mode. |
I haven't needed to short the BOOT pins in a long time... does the DFU alternate interface not work on G0 ? |
DFU likely does work in general, however I think there is a bug in the mks_utc board, so it does not work there. |
Just implemented the missing bits in |
I've pushed the dfu changes. |
@marckleinebudde merged your changes and took a stab at the LED /channel integration. Match vendor ID from file: 1d50 the board does not reset/reboot, does it need a button to trigger DFU mode? |
No (Haven't tried with G0 however). Does I normally don't use the " make flash-...." target since I always have more than one board connected and therefore need to invoke dfu-util manually to specify serial number. |
DFU is working correctly. I'm stupid and did not update the udev rules. CAN functions were working fine, just the DFU could not open the device. Updated the udev rules and works like a charm. |
Thanks for the work, I've polished it a bit and added to my https://github.com/marckleinebudde/candleLight_fw/tree/multichannel branch. |
@marckleinebudde I figured out what I thought is a layout issue with my board. On some transceivers one has to use the controller's delay compensation. I added this to the board config structure as it seems to be board dependent. My board works fine at 1/5 Mbps now. |
I also re-worked the clock config on the G0. The default 40MHz CAN clock is a bit low when you get into high speeds. I managed to squeeze 80MHz CAN clock / 64MHz cpu clock. |
TDC is bitrate dependent. See https://elixir.bootlin.com/linux/v6.2.5/source/drivers/net/can/m_can/m_can.c#L1204 |
20 or 40 MHz are the CIA recommended values for the CAN clock. I have a peak USB adapter that uses 80 MHz, though. Which problems do you see with 40 MHz only? These are the default timing parameters calculated by the algorithm in the Linux Kernel:
|
Howdy STM32Gxx folks - at present is it best to use this PR's source branch/fork - or the BTT fork without a PR for STM32G based devices such as the EBB36? |
Hey @sammcj, my Work-in-Progress stm32g0b1 branch is here: https://github.com/marckleinebudde/candleLight_fw/tree/multichannel. It contains the newest g0b1 support. |
@marckleinebudde Thank you! I was hesitant to use BTT's compiled binary firmware as they did not provide any explanation of how the binary was created. I'm able to confirm that your branch with "make nucleo_g0b1re_fw" works perfectly with the BTT U2C v2.1 board. Do you have a rough timeline of when this will be merged into mainline? |
Were you able to validate that 2 CAN Bus interfaces work on the BTT U2C? |
Your board might need a different pinmux, if so create a new board and send a PR against my branch. |
I'm not happy with the LED stuff, yet. And @fenugrec has to have a look at the board abstraction. 😸 |
I did see 2 interfaces when running |
That proofs that the device announces 2 devices. So the the µC firmware and kernel are working together. But it does not show, that the µC is configured properly towards the hardware...
Can you use the other connector on the board and configure |
Another simple test method is to connect the busses together and validate tx from one to the other and vice versa. |
OK. Still wiring things up. Will do both tests once I get the chance. |
Oops, I thought the huge pile in #139 was still essentially 'WIP, don't merge' and I took advantage of the intervening time to work on other projects. (one of those, trying to get reasonable performance on windows, has proven to be one of the most infuriating things I've ever worked on, and left me completely discouraged about USB in general) Board abstractions : are you referring to a specific set of commits that I may be overlooking ? Else, I know I've expressed the intention of working on that (had some kind of rework in mind) but unfortunately I've had to greatly reduce the amount of coding I do. Reviewing, testing and merging is still fine though. |
Oh dear, luckily I don't have to work on Windows....
|
I'm looking at 048601f...1c6e7cb , mostly OK but I don't think that range can be cherry-picked as-is ? e.g. it depends on earlier commits adding |
Did this ever get updated and merged in another PR? I'm still running off this branch 😅 |
How do I build for STM32G4? I don't see anything obvious in |
Is building from this branch still required? |
There are a lot of commits with this PR which will make @marckleinebudde happy.
I've broken the changes down into bite sized parts.
The last commit is the one I need the most help with. Please read the notes on the commit.
I confirmed that the code works on my G0 hardware and I had one old canable device (F042) here that I was able to load and verify that the USB functionality works but I wasn't able to test CAN due to my soldering skills being poor when I built it about 2 years ago.