can: Add Linux compatible frame and filter structs

[jukkar]

Add new Linux compatible "struct can_frame" so that it is easier to
port socket-can application from Linux.
Add also can_id and can_mask fields to can_filter struct as those
names are found in Linux.

Signed-off-by: Jukka Rissanen [email protected]

[alexanderwachter]

I think you forgot the reordering to be bit compatible:

/** Message identifier*/
union {
	u32_t std_id  : 11;
	u32_t ext_id  : 29;
};
/** Set the message to a transmission request instead of data frame
 * use can_rtr enum for assignment
 */
u32_t dummy : 1;
u32_t rtr         : 1;	
u32_t id_type : 1;

[codecov-io]

Codecov Report

Merging #13374 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #13374      +/-   ##
==========================================
+ Coverage   52.41%   52.43%   +0.02%     
==========================================
  Files         322      323       +1     
  Lines       46596    46622      +26     
  Branches    10772    10772              
==========================================
+ Hits        24422    24448      +26     
  Misses      17267    17267              
  Partials     4907     4907

Impacted Files	Coverage Δ
include/can.h	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2d5e7b...3de6f66. Read the comment docs.

[nashif]

Are you copying anything from Linux here?

[alexanderwachter]

Alternative version:

#define CAN_EFF_POS 31
#define CAN_EFF_FLAG (1 << CAN_EFF_POS)
#define CAN_RTR_POS 30
#define CAN_RTR_FLAG (1<< CAN_RTR_POS)

#define can_msg can_frame
typedef u32_t canid_t;

struct can_frame {

	union {

		struct {
			union {
				u32_t std_id  : 11;
				u32_t ext_id  : 29;
			};
			u32_t dummy   : 1;
			/** Indicates the identifier type (standard or extended)
			 * use can_ide enum for assignment
			 */
			u32_t rtr     : 1;
			/** Message identifier*/
			/** The length of the message (max. 8) in byte */
			u32_t id_type : 1;
			/** Set the message to a transmission request instead of data frame
			 * use can_rtr enum for assignment
			 */
		};
		canid_t can_id;
	};
	union {
		u8_t dlc;
		u8_t can_dlc;
	};
	
	/** The message data*/
	union {
		u8_t data[8];
		u32_t data_32[2];
	};
} __packed;

[jukkar]

This looks a bit confusing actually, the original can_msg has the fields in wrong order when I compare them to https://en.wikipedia.org/wiki/CAN_bus#Frames. Also the extended format description in wiki does not match what I see in can_msg.

The length of the struct can_msg looks wrong in quick glance as it does not seem to match wire format. For example the first union is always 32-bit length even if standard addressing is used which according to wiki is only 18-bits long. To me it looks like that the can_msg is not actually a wire format presentation of the CAN message but it has the __packed in it which is a bit confusing actually.

[jukkar]

Are you copying anything from Linux here?

Only the struct and field names for CAN message. No code or anything like that.

[alexanderwachter]

@jukkar can_msg is only a container to unify messages around all can controllers.
__packed is a leftover from my tests to shrink the struct. It's save to remove.

[alexanderwachter]

@jukkar basically you can do whatever you want with the struct can_msg, as long as the names don't change. If the names change we break all existing code. But if we want to change them we shoud do it now. I'm not sure but I think CAN just stats to be used and the damage is limited at the moment.

we could do this along with #13262

[alexanderwachter]

We should also fix the error messages like CAN_TX_*. The have positive numbers.
Sorry about that. I didn't know what I was doing at this time ;-)

[jukkar]

Ok, this clarifies things. I had the impression that the can_msg is in wire format as it had the __packed attribute. I will remove it in next version of this PR.

[jukkar]

Updated the code. Introduced can_frame and can_filter structs so that we can compile Linux based CAN code with SocketCAN. This also means that the existing can_filter needs to be renamed to can_msg_filter in order not to conflict with Linux compatible version.

One possible route could be to rename existing zephyr specific structs from can_msg -> zcan_frame and can_filter -> zcan_filter.

I also created unit tests for testing helper functions I introduced in this PR.

[zephyrbot]

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

[alexanderwachter]

@jukkar I like the idea with zcan_*
zcan_frame for can_msg, zcan_filter for can_filter

[jukkar]

zcan_frame for can_msg, zcan_filter for can_filter

This could be done, it is just changing the CAN API but we needed to change that anyway for Linux compatibility. Should we also change the function names at the same time so can_*() -> zcan_*(), what do you think?

[alexanderwachter]

I don't think that we get collisions for the API wrapper names here.
Other drivers like i2c or SPI also don't have a z in their API.

[pfalcon]

Thanks for dealing with this, let me give +1, I hope you guys will decide whether further renamings make sense.

[lowlander]

When you are going to change the CAN API anyway, I still am working on drivers for the F4 (dual CAN) and E70 CAN-FD experimental stuff (OK E70 is in very early development) is here ;

lowlander@9c87c60

One of the things I did was using Linux style canid_t instead of the union style.

For CAN-FD I added extra RX and TX API calls.
The RX filter API I changed to pass an array of filters and to pass FIFO-nr info. The array is directly used so it can be in RO-memory and does not have to be copied (reducing RAM, the F4 can have upto 112 filters, and the F7 upto 224 filters for 3 ports, which would use several kbyte RAM when copied in the driver)

I removed the kqueue support, because I do not want to implement that in every driver, I believe the kqueue is something to generally implement in a higher layer for all drivers in one go.

Made the sending a sync call (there are very small hardware queues anyway, just 3 mboxes on the F4) so other queuing can be done by higher level code.

Other changes are the at runtime calculation of the sample point, instead of using hardcoded DT values.

All in all a lot of changes, that I wanted to really clean up and (especially for CAN-FD) do some more private testing with before creating a PR. Sadly I do not have a huge amount of time for Zephyr work so things are a bit slow, would like to upstream it at some point, but it is simply not ready yet.

Just my few cents to the CAN topic.

[alexanderwachter]

@lowlander can you make an own issue for that? The filter are marked as const in the actual implementation an it's save to do whatever you want with them after you called attach*. They don't need any memory except to save the callback (that must be changeable at run-time). The attach_msgq will be changed to a generic version after #13262 is merged.

[lowlander]

@lowlander can you make an own issue for that?

Not until I am OK with the quality, I got bad experience with trying to "develop" with github PRs, because ppl seem to focus on "coding style" and not on the actual content.

The filter are marked as const in the actual implementation an it's save to do whatever you want with them after you called attach*. They don't need any memory except to save the callback (that must be changeable at run-time).

And the void* user argument, which already make 8 byte per filter, with upto 112 filter that is ~1Kbyte, of duplicated data. Also the driver must be instructed on how big that filter table can be via menu config. For drivers that do not have hardware filtering it would be even worse, because than you also need the actual filter data (id and mask, adding another 8 bytes per entry).

Also by using a user defined table the order of the filters is more clearly defined.

For "dynamic" filters the subsystem (like socketCAN) can just create a table in RAM fill that and assign that, as long as it makes sure it stays valid for the use time. To update a table like that register a NULL table update the RAM table and reregister it. (that way one could also create a compatibility layer that mimics the current API)

The attach_msgq will be changed to a generic version after #13262 is merged.

To be honest I think CAN should have been marked [EXPERIMENTAL] and allowed big API changes, cause their seem to be missing several things that a fixed (backwards compatible) API make very hard to fix.

I'll see how well I can integrate my stuff into upstream, if it doesn't work out I am fine with that too.

[jukkar]

As discussed in the PR, I added a new commit that renames existing struct names. So can_msg would become zcan_frame, and can_msg_filter would become zcan_filter.

[nashif]

need to fix the native_posix failures.

[galak]

@jukkar this change didn't seem to update the tests in tests/drivers/can/ to match.