[Core]: Add more concrete ways of sending data

[GwnDaan]

Ultimately, the goal would be to be able to pass in any data structure to send_can_message, and it will figure out how to send the data. This PR should be a good base for that vision. The DataSpan class allows us to wrap any data structure and 'view' it's contents.

What's changed:

• DataSpan class
• Separated calls with 'data pointer' and 'chunk callback' parameters. There won't be a use case where both are used at the same time
• Added various other data structures to be allowed as parameters to the send_can_message function.
• Cleaned up the send_can_message function

Future plans:

• reference to std::vector as data structure
• reference to std::list as data structure
• combining 'dataspan' and 'chunk callback' by using a common interface between the two

[ad3154]

Separated calls with 'data pointer' and 'chunk callback' parameters. There won't be a use case where both are used at the same time

This seems OK to me conceptually. Maybe its a bit easier to read and maintain, plus fewer arguments for people to think about.

But, I'm not really bought into much of this for a few reasons...

1. Adding a function for explicitly sending to the global address slightly reduces the abstraction away from addresses to CFs and PGNs. One of my main goals has always been to minimize the need for the awareness of addresses at all.
2. Adding a function for sending to global doesn't reduce the chance of failure. A user could easily call send_can_message_global with a PGN of EF24 and it would have to fail, same as now where if they tried to send EFFF with a destination it would fail. So I don't think it really gains us anything.
3. As long as we only have DataSpan<const std::uint8_t> I don't think it really gains us anything except being able to also accept std::array without the user doing .data() and .size(). Seems like not enough to start deprecating parts of our public API.
4. Adding the Data span adds confusion for the user I think if we add more than just uint8_t containers as valid types. I am not sure if this exact scenario is in your intention for this, but, for example, if down the road someone passes in a vector of float or uint16_t because they are trying to send a UDS message (ISO 14229) or something, the endianness is backwards from ISO11783 if I recall correctly, but explicitly tolerated on an ISO11783 bus. So chances are high we'd corrupt their CAN message, which may not be clear to them or desirable from our public interface. Likewise, most people I've seen using other CAN stacks also only use either uint8_t *, vector<uint8_t>, or unsigned char *, so it's what people are probably familiar with, and if we're stuck with uint8_t I'm back to "I don't know how much this really gains us".
5. This one is just personal preference but I like using standard types and not wrapper templates where possible... I think it reduces the learning curve for people with less C++ experience and minimizes the binary size.

[GwnDaan]

Hmm, I think all your points are valid. Let's have a little discussion haha

1. Adding a function for explicitly sending to the global address slightly reduces the abstraction away from addresses to CFs and PGNs. One of my main goals has always been to minimize the need for the awareness of addresses at all.

I agree, and that should still be the main goal. How it currently is, is that there is a function where destination is optional, meaning that someone may forget to put a destination, or not understanding. Though I agree this is pretty unlikely, it was the sole reason why I added the _global function and made the destination required for the normal one. I'm fine with removing it if you still think it is unnecessary.

2. Adding a function for sending to global doesn't reduce the chance of failure. A user could easily call send_can_message_global with a PGN of EF24 and it would have to fail, same as now where if they tried to send EFFF with a destination it would fail. So I don't think it really gains us anything.

Yes, that's true, it was simply a synonym for the normal one but then without a destination. It might only give us better user experience if my reasoning above holds for some people haha.

3. As long as we only have DataSpan I don't think it really gains us anything except being able to also accept std::array without the user doing .data() and .size(). Seems like not enough to start deprecating parts of our public API.

Okay, deprecating parts of the public API is maybe too much indeed and can be removed. The main reason for DataSpan is that you make sure after construction of the Span, the pointer and size never changes. Hence it's integrity is held after passing it around everywhere. We can't make mistakes altering the data, since we implicitly can't. So by converting any data structure into the DataSpan using it's factory methods, we only have to test the factory methods once, and we are certain that everywhere the data is still unchanged. Ideally, I would've used std::span but that is only available with C++20.

Adding the Data span adds confusion for the user I think if we add more than just uint8_t containers as valid types. I am not sure if this exact scenario is in your intention for this, but, for example, if down the road someone passes in a vector of float or uint16_t because they are trying to send a UDS message (ISO 14229) or something, the endianness is backwards from ISO11783 if I recall correctly, but explicitly tolerated on an ISO11783 bus. So chances are high we'd corrupt their CAN message, which may not be clear to them or desirable from our public interface. Likewise, most people I've seen using other CAN stacks also only use either uint8_t , vector, or unsigned char , so it's what people are probably familiar with, and if we're stuck with uint8_t I'm back to "I don't know how much this really gains us".

True, it's probably better to refactor it to CANDataSpan instead that only allows for std::uint8_t values.

This one is just personal preference but I like using standard types and not wrapper templates where possible... I think it reduces the learning curve for people with less C++ experience and minimizes the binary size.

I see, I can change that to just be std::uint8_t instead, if we still want any of the changes hahah. Let me know 😄

[GwnDaan]

Oh and, basically the main reason I did these changes is to facilitate:

combining 'dataspan' and 'chunk callback' by using a common interface between the two

What are your thoughts on that? It would be really nice to also use it higher up in the transport protocols I think

[sonarcloud[bot]]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

57.9% Coverage
0.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint