-
Notifications
You must be signed in to change notification settings - Fork 130
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
Feature 64-byte FDCAN messages #882
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.
Nice work, thank you a lot!! :)
It does not really hurt performance or memory usage if it is not used, so I'm fine with having
Nice 👍🏽
I don't like this so much because I know some applications where the FDCAN IP is only used with traditional 8 byte messages and the larger buffers will probably cause problems. Maybe we can have an lbuild option (per FDCAN instance) to enable the long messages? If the option defaults to |
Thanks for your comments! I will make the code backward compatible. How do you suggest we do this? What seems to cause the problem is the depreciation of the length variable. For CAN the dlc and length is the same and hence for backward compatibility we could rename the dlc variable to length? I personally would prefer not to have both a dlc and a length variable. Also I much prefer using the dlc because it reduces the conversion to looking up in an array! Also I like the idea of passing the message buffer length to lbuild. Then I suggest we remove the message base class template and set the message buffer size with lbuild so we don't have double templates. For example we add an option What do you think? |
I have made the change so the message buffer size can be set in project file. However, the change is global across all fdcan lines. I think it would be a nice addition to be able to specify it for the particular line. I can implement this by reintroducing the template parameter and allow the specific fd can line to send every message with buffer length less than or equal to the specified message buffer size. What do you think about that? One should be very aware that the message used by the specific can line would then end up in the specific can line namespace with this approach e.g. Fdcan1::message. |
6d74372
to
488e19b
Compare
I consider CAN FD done and use it for my own project already. What are your thoughts? |
Sorry for the long delay, I was busy working on other projects. |
Thank you! Let me know if you think further changes are necessary? |
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.
Nice work!
@rasmuskleist Do you want to squash and rebase your commits? |
488e19b
to
e91d684
Compare
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.
Thanks!
@rasmuskleist Thank you very much for all the work and sorry review took such a long time. |
@chris-durand sorry for stagnating the development of this PR! I have been exceptionally busy with my thesis. I am currently facing some issues when the header type of the message is not extended. When extended everything works fine, however, when not extended it seems that the message is being sent but not received. Until now I have not resolved the issue but plan to do so in the near future. Have you got any ideas as to why this might be the case? |
Sounds to me like the CAN message filter is not configured correctly. |
Turned out it was the filter... from my part this pr is ready for merging now |
899af57
to
5fe4e8c
Compare
@rasmuskleist @twast92 There is still a small issue left before we can merge. The CAN message header fails to compile on Mac OS:
The matching overload of |
@twast92 That worked, thanks. Please squash the fix commit and we'll merge. |
e9ac004
to
fd6bea2
Compare
fd6bea2
to
e4b1a4a
Compare
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.
Thanks!
The code has been tested on a custom board using the stm32g491met6 chip. There are a few points that should be considered.
I would like to hear your thoughts on these problems and if there would be other considerations?