CAN-FD support take 2 #103

kjetilkjeka · 2019-03-26T20:47:04Z

@pavel-kirienko see if this is something more in the lines of what you want

pavel-kirienko · 2019-03-27T17:31:42Z

canard.c

@@ -132,6 +132,7 @@ uint8_t canardGetLocalNodeID(const CanardInstance* ins)
 }

 int16_t canardBroadcast(CanardInstance* ins,
+                        CanardTransportProtocol protocol,


I propose we set the protocol once during initialization (new argument for canardInit()). There is no point in selecting the protocol more than once at runtime, is there? Having it specified once simplifies both the API and internal calls, e.g. enqueueTxFrames().

This is required for CAN2.0B and CAN-FD to co-exists with the same interface. You need to set the one you want libcanard to generate frames for.

I cannot see any other way to get broadcast with both frames as an option to work

This is required for CAN2.0B and CAN-FD to co-exists with the same interface. You need to set the one you want libcanard to generate frames for.

True, but why would one want to set it more than once (i.e., after initialization)? The use case I imagine is as follows: an integrator/engineer takes a third-party UAVCAN node and integrates it into their vehicle. Say, the node utilizes libcanard and supports CAN FD (and therefore CAN 2.0, too). The factory-default configuration tells the node to use CAN FD (because it's superior), but the vehicle it is being integrated into uses CAN 2.0, so the engineer connects to the node and changes its configuration from CAN FD to CAN 2.0. That's it.

This is the main use case I had in mind when we were talking about runtime protocol selection. Did you have a different use case in mind?

Yes. Let's say you have a CAN-FD compatible UAVCAN node in a network which are communicating with other CAN-FD compatible UAVCAN nodes. They will, therefore, encode all UAVCAN messages into CAN-FD frames up to 64 Bytes. However, a node with a CAN2.0 controller may still coexist on the same network as long as the transceiver is CAN-FD tolerant. When receiving a CAN-FD frame the transceiver will simply "ignore" it and not pass it forward to the controller. When a UAVCAN node is configured to operate in CAN-FD compatible mode it should still be possible to transmit UAVCAN messages to other non-CAN-FD compatible nodes which may exist on the same network.

It is, therefore, required to select between using CAN-FD or CAN2.0 in runtime.

The dynamic selection will be useful only if all of the following criteria are met:

there are CAN 2.0 and CAN FD nodes mixed inside the same network

all CAN 2.0 nodes use CAN FD capable drivers

CAN FD capable nodes know which nodes on the bus are restricted to CAN 2.0

The cost of the dynamic selection feature is a marginal increase in the complexity of the library API a minor increase in the ROM footprint.

My assessment is that the described use case would be exceedingly rare to encounter in the field, in which case the cost of supporting it may make it unworthwhile. Would you not agree?

The flex array members (the one that broke the c specc) is easily fixable. Flex array members themselves should be fine to use in this type of application.

The extra operations you're talking about is basically one subtract operation. I don't think there will be any significant difference, if you're very concerned about this we must benchmark it.

Setting block size at init allows us to only produce and test one binary. And doesn't mess up memory usage when compiling with fd support.

The only other decent alternative is compiling for fd only or 2.0 only. Then there is no dynamic block size, but we must test two binaries.

Im against implementing compatibility mode if we're not aiming for simultanous use of different frames. Which i didn't think we needed in the first place.

Performance is on my list of concerns but only barely, just like memory usage.

Would I be correct at describing your proposed approach with the following table?

Capability Inited in CAN FD mode Inited in CAN2 mode

Transmission CAN FD only CAN 2.0 only

Reception Any Any

Pool block size 88 bytes 32 bytes

Max payload size 1 KiB 1 KiB

Observe that the ability to emit only CAN 2.0 while being able to receive both CAN 2.0 and CAN FD is also paramount for bootloaders, which must be able to operate while not knowing whether the network they are attached to is CAN FD or CAN 2.0.

Would I be correct at describing your proposed approach with the following table?

Yes, this is what I had in mind.

Observe that the ability to emit only CAN 2.0 while being able to receive both CAN 2.0 and CAN FD is also paramount for bootloaders, which must be able to operate while not knowing whether the network they are attached to is CAN FD or CAN 2.0.

What if someone connects them to I2C? I don't really get why bootloaders are special in this regard.

I think configuring this dynamically would be the sensible approach. Selecting block size dynamically allows configuring at run-time between CAN2.0B and CAN-FD without wasting resources.

I still feel dynamically chosen block size at initialization is the technically superior alternative. But the "chosen at compile time" alternative is still OK with me.

If we go this route there is a lot of inconveniences we don't really need to have, some of them are:

Cannot configure a node between 2.0 and FD at run time (not even bootloaders), without compiling both versions of the library.

Cannot connect to multiple buses with different protocols without compiling both versions of the library.

A vendor cannot support CAN2.0 and CAN-FD without compiling both versions of the library.

Remember that my original PR was a WIP, and a lot of the weirdness seen in it can be removed.

What if someone connects them to I2C? I don't really get why bootloaders are special in this regard.

Bootloaders are special because you can't configure them. A bootloader must be able to operate on a bus while having zero prior knowledge of its properties (other than it's UAVCAN-compliant) - whether it's CAN 2.0 or CAN FD, its bit rate, the node ID of the local device, etc. A bootloader that is dependent on correct configuration complicates the update process and makes the system far less brick-proof than it could be. We researched this question with the PX4 team a few years back while building the PX4 Brickproof Bootloader.

Regardless, though, if we are able to receive both CAN FD and CAN 2.0 while operating in the CAN 2.0 mode (which seems to be the case in all of the discussed alternatives so far), we're okay and there is nothing more to discuss.

If we go this route there is a lot of inconveniences we don't really need to have, some of them are:

Surely we can switch between CAN FD and CAN 2.0 at run time (e.g., during initialization) if the block size is large enough. Selecting CAN 2.0 in that case comes with suboptimal memory usage, as we discussed earlier. If we are okay with wasting memory, none of the listed inconveniences seem to exist.

I am not totally against the runtime-selectable block size. If you want it done this way, it certainly can be; we can go back to the original PR.

pavel-kirienko · 2019-03-27T17:35:21Z

canard.c

@@ -62,7 +62,7 @@
 struct CanardTxQueueItem
 {
    CanardTxQueueItem* next;
-    CanardCANFrame frame;
+    CanardTransportFrame frame;


I am not really objecting here, just asking: is the renaming from CanardCANFrame to CanardTransportFrame intentional, or is it just a leftover from the previous PR? Since both protocols are variants of CAN, the old name would have been fine, too.

It was intentional. I've started to refer to the "new" one as FD_FRAME. So now we got "CAN", "FD" and "TRANSPORT" for both. Do you have any suggestions for better naming?

I think that when we say "CAN" we mean either "CAN 2.0" or "CAN FD", would you not agree? The taxonomy would be like this:

Transport

CAN <-- we are here

CAN 2.0B

CAN FD

UDP

IEEE 802.14

pavel-kirienko · 2019-03-27T17:36:10Z

canard.c

+        }
+        default:
+        {
+            return -CANARD_ERROR_INVALID_ARGUMENT;


How about adding CANARD_ASSERT(false) before the return statement? Also, the following break is unreachable, many an IDE would complain.

My bad attempt at actually following MISRA, willfix

MISRA is not self-contradictory here. It also says that there must be only one return statement per function, which can be difficult to follow; if you have to exit early, you may also be forced to break other rules, like having at least one break per case statement.

pavel-kirienko · 2019-03-27T17:38:10Z

canard.c

+            if (data_len <= 8)
+                next_available_dlc = data_len;
+            else
+                next_available_dlc = 8;            


Missing braces { }.

Didn't realize they were mandatory with the guildelines, I'll add them. I must say that with zubax C guidelines this code particular, and switch case in generals, isn't exactly visual appealing.

isn't exactly visual appealing.

Hey, it's C ¯\_(ツ)_/¯

Also, see MISRA C++ 6-4-1:

pavel-kirienko · 2019-03-27T17:38:21Z

canard.c

+            else if (data_len <= 48)
+                next_available_dlc = 48;
+            else
+                next_available_dlc = 64;


Missing braces { }.

pavel-kirienko · 2019-03-27T17:38:56Z

canard.c

+        }
+    }
+
+    for (int i = data_len; i <= next_available_dlc; i++)


Please avoid reliance on the fundamental types (MISRA recommendation), use uint8_t instead.

willfix, thanks

pavel-kirienko · 2019-03-27T17:41:03Z

canard.h


 /// The size of a memory block in bytes.
-#define CANARD_MEM_BLOCK_SIZE                       32U
+#define CANARD_MEM_BLOCK_SIZE                       88U


But this should be selectable at compile time? Like,

#if CANARD_TRANSPORT_PROTOCOL == CanardTransportProtocolCANFD # define CANARD_MEM_BLOCK_SIZE 88U #else if CANARD_TRANSPORT_PROTOCOL == CanardTransportProtocolCAN2 # define CANARD_MEM_BLOCK_SIZE 32U #endif

canard.h

pavel-kirienko · 2019-03-27T17:45:06Z

canard.h

@@ -118,8 +124,15 @@ extern "C" {
 #define CANARD_MAX_TRANSFER_PAYLOAD_LEN             ((1U << CANARD_TRANSFER_PAYLOAD_LEN_BITS) - 1U)


+typedef enum
+{
+    CanardTransportProtocolCAN,


Suggested change

CanardTransportProtocolCAN,

CanardTransportProtocolCAN2,

CAN2 is short for "CAN 2.0".

great! willfix

pavel-kirienko · 2019-03-27T17:45:26Z

canard.h

+typedef enum
+{
+    CanardTransportProtocolCAN,
+    CanardTransportProtocolFD,


Suggested change

CanardTransportProtocolFD,

CanardTransportProtocolCANFD,

great! willfix

kjetilkjeka · 2019-03-27T18:43:29Z

@pavel-kirienko I take your review as I may proceed in this direction. Let me know if I should interpret it otherwise.

ntakouris · 2019-03-31T15:14:43Z

The idea and implementation seems good to me. I just want to comment this:

I'd prefer this to be only a compile-time configuration. Having to dynamically reconfigure on runtime protocol switch sounds like a weird edge case. I am not fond of having to mix and match both CAN standard versions on the same bus, although that is a nice feature to have and makes the use of UAVCAN progressive and easy to integrate to an existing solution.

You guys have more experience than me on this, but were there any field tests performed that proved that this change is necessary? Did you encounter some annoyingly lack of memory issues to back up the extremely wasteful with memory claim on #96 ?

This reminds me of the same thing that @thirtytwobits said in this weeks devcall: You might have some extra tiny MCU just to transmit sensor values in the bus. If these nodes are only transmitters, would it make sense to refactor libcanard to have even tighter resource footprints ?

P.S. Formula Student is lit

pavel-kirienko · 2019-03-31T19:09:01Z

As we discussed above, the ability to choose the protocol at runtime is absolutely critical for bootloaders and generic nodes which must be usable with CAN 2.0 and CAN FD networks alike. The industry is not going to switch to CAN FD overnight.

The memory issue can be seen as a trade-off between complexity and memory footprint. I am open to whatever solution Kjetil finds more appropriate. #103 (comment)

fvantienen · 2019-05-08T07:10:05Z

Is this currently in a working state? Because I want to use it in my project.

pavel-kirienko · 2019-05-08T12:38:06Z

Not quite, but it's close. Are you keen on using UAVCAN v1.0?

…

On Wed, May 8, 2019, 10:10 Freek van Tienen ***@***.***> wrote: Is this currently in a working state? Because I want to use it in my project. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#103 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZFIZF5KHDSSBXWNIYUQVDPUJ4E5ANCNFSM4HBRVECQ> .

fvantienen · 2019-05-08T14:23:43Z

Yes I am

pavel-kirienko · 2019-05-08T16:30:31Z

@kjetilkjeka do we have any advice for the gentleman?

pavel-kirienko · 2019-09-12T07:42:43Z

@kjetilkjeka should I close this for #134?

Added support for CAN-FD

7fbbefc

kjetilkjeka mentioned this pull request Mar 26, 2019

[WIP] Multiple transport protocol (CAN-FD) support #96

Closed

pavel-kirienko added this to the v1.0 milestone Mar 27, 2019

pavel-kirienko requested changes Mar 27, 2019

View reviewed changes

pavel-kirienko closed this Jan 9, 2020

pavel-kirienko mentioned this pull request Mar 3, 2020

UAVCAN v1.0 done #142

Merged

Capability	Inited in CAN FD mode	Inited in CAN2 mode
Transmission	CAN FD only	CAN 2.0 only
Reception	Any	Any
Pool block size	88 bytes	32 bytes
Max payload size	1 KiB	1 KiB

CAN-FD support take 2 #103

CAN-FD support take 2 #103

Conversation

kjetilkjeka commented Mar 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjetilkjeka commented Mar 27, 2019

ntakouris commented Mar 31, 2019

pavel-kirienko commented Mar 31, 2019

fvantienen commented May 8, 2019

pavel-kirienko commented May 8, 2019 via email

fvantienen commented May 8, 2019

pavel-kirienko commented May 8, 2019

pavel-kirienko commented Sep 12, 2019