Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Feature 64-byte FDCAN messages#882

Merged
salkinium merged 1 commit intomodm-io:developfromDanSTAR-DTU:feature/fdcan
Feb 4, 2023

Conversation

rasmuskleist
Copy link
Contributor

The code has been tested on a custom board using the stm32g491met6 chip. There are a few points that should be considered.

  1. The LongMessage is also currently available to devices only supporting CAN. Naturally guards can be implemented in lbuild to only generate LongMessage if supported. However, only Fdcan{{ id }} implements methods for sending LongMessage.
  2. LongMessage has a 64-byte buffer but there is not directly implemented support for other buffer sizes like 32-byte. The MessageBase class is implemented such that the user can specify the buffer size accordingly.
  3. The message queue now per default contains LongMessages when using FDCAN. Thereby the buffer is approximately eight times bigger as compared to CAN. Possibly it would make sense to change the default fdcan buffer size to accommodate this.
  4. The FDCAN test is effectively the same as the can test for the nucleo g474. Hence both regular CAN and FDCAN could be tested in the original test file.

I would like to hear your thoughts on these problems and if there would be other considerations?

TomSaw and rleh reacted with heart emoji
Copy link
Member

@rlehrleh left a 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!! :)

@rleh
Copy link
Member

  1. The LongMessage is also currently available to devices only supporting CAN. Naturally guards can be implemented in lbuild to only generate LongMessage if supported.

It does not really hurt performance or memory usage if it is not used, so I'm fine with havingLongMessage always available.

  1. LongMessage has a 64-byte buffer but there is not directly implemented support for other buffer sizes like 32-byte. The MessageBase class is implemented such that the user can specify the buffer size accordingly.

Nice 👍🏽

  1. The message queue now per default contains LongMessages when using FDCAN. Thereby the buffer is approximately eight times bigger as compared to CAN. Possibly it would make sense to change the default fdcan buffer size to accommodate this.

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 tofalse this PR should also be perfectly backward compatible.

@rasmuskleist
Copy link
ContributorAuthor

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
<option name="modm:can:message.buffer_size">64</option>
or equivalent. Thereby we will only have to change the message code and there will be no need differentiate between Message and LongMessage in the CAN code. With this approach we should decide if we want to guard if the input given in the project file makes sense. For example so it's contained within the list of possible message lengths. However, passing a number would at least not break the code.

What do you think?

@rasmuskleist
Copy link
ContributorAuthor

rasmuskleist commentedJul 14, 2022
edited
Loading

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.

@rlehrleh self-requested a reviewAugust 10, 2022 13:17
@salkiniumsalkinium removed this from the2022q3 milestoneOct 1, 2022
@twast92twast92force-pushed thefeature/fdcan branch 2 times, most recently from6d74372 to488e19bCompareOctober 6, 2022 11:58
@rasmuskleist
Copy link
ContributorAuthor

I consider CAN FD done and use it for my own project already. What are your thoughts?

@rleh
Copy link
Member

rleh commentedNov 6, 2022

Sorry for the long delay, I was busy working on other projects.
I have already checked out your branch locally on will review it later or tomorrow.

@rasmuskleist
Copy link
ContributorAuthor

Thank you! Let me know if you think further changes are necessary?

Copy link
Member

@rlehrleh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice work!

@rleh
Copy link
Member

rleh commentedNov 6, 2022

@rasmuskleist Do you want to squash and rebase your commits?

Copy link
Member

@chris-durandchris-durand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks!

@chris-durand
Copy link
Member

chris-durand commentedJan 31, 2023
edited
Loading

@rasmuskleist Thank you very much for all the work and sorry review took such a long time.
We intend to merge the changes very soon. Since I don't have permission to push to your branch I'll open a new pull request soon with the commits cleaned up a bit and we'll merge shortly.

salkinium and rasmuskleist reacted with thumbs up emojisalkinium and rasmuskleist reacted with heart emojisalkinium and rasmuskleist reacted with rocket emoji

@rasmuskleist
Copy link
ContributorAuthor

@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?

@rleh
Copy link
Member

rleh commentedFeb 1, 2023
edited
Loading

Sounds to me like the CAN message filter is not configured correctly.

@rasmuskleist
Copy link
ContributorAuthor

Turned out it was the filter... from my part this pr is ready for merging now

@chris-durand
Copy link
Member

@rasmuskleist@twast92 There is still a small issue left before we can merge. The CAN message header fails to compile on Mac OS:

modm/src/modm/architecture/interface/can_message.hpp:193:37: error: no matching function for call to 'begin(const uint8_t [8])'  193 |                 std::copy(std::begin(rhs.data), std::end(rhs.data), std::begin(this->data));

The matching overload ofstd::begin is not found. This looks like an include problem. On other compiler versions / platforms the right file gets included indirectly somewhere. Could you try adding#include <array> in the header file and see if it resolves the CI error? Otherwise we should be ready to merge.

twast92 reacted with eyes emoji

@chris-durand
Copy link
Member

@twast92 That worked, thanks. Please squash the fix commit and we'll merge.

twast92 and rasmuskleist reacted with hooray emoji

@rlehrleh added this to the2023q1 milestoneFeb 3, 2023
@chris-durandchris-durand added the ci:halTriggers the exhaustive HAL compile CI jobs labelFeb 3, 2023
Copy link
Member

@salkiniumsalkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks!

@salkiniumsalkinium merged commite4b1a4a intomodm-io:developFeb 4, 2023
@twast92twast92 deleted the feature/fdcan branchFebruary 4, 2023 14:38
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@salkiniumsalkiniumsalkinium approved these changes

@rlehrlehrleh approved these changes

@chris-durandchris-durandchris-durand approved these changes

Assignees
No one assigned
Labels
advanced 🤯ci:halTriggers the exhaustive HAL compile CI jobsfeature 🚧
Milestone
2023q1
Development

Successfully merging this pull request may close these issues.

4 participants
@rasmuskleist@rleh@chris-durand@salkinium

[8]ページ先頭

©2009-2025 Movatter.jp