Feature 64-byte FDCAN messages

[rasmuskleist]

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?

[rleh]

Nice work, thank you a lot!! :)

[rleh]

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 having LongMessage always available.

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.

Nice 👍🏽

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.

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

[rasmuskleist]

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]

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.

[rasmuskleist]

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

[rleh]

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]

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

[rleh]

Nice work!

[rleh]

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

[chris-durand]

Thanks!

[chris-durand]

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

[rasmuskleist]

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

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

[rasmuskleist]

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

[chris-durand]

@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 of std::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.

[chris-durand]

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

[salkinium]

Thanks!