Can Monitoring

[raffael0]

This PR works in conjunction with the PRs in firmware_liquids, STRHAL and LLServer.
It adds a new channel which monitors the can state of the ECUs

[miDeb]

Looks good, just the alignment of the registers in the struct needs to be fixed.

[miDeb]

Suggested change

	uint32_t RES : 1; // Bit 14 - Reserved
	uint32_t RES : 1; // Bit 15 - Reserved

[miDeb]

Also since this is a new file please use a consistent formatting (just noticed some lines are longer/shorter and use tabs/spaces)

[miDeb]

This looks like it is not working as intended because the compiler does not necessarily lay out the raw struct with psr coming right after ecr, instead giving both psr and ecr their own 32 bits each when inserting padding. It then doesn't line up with how the bits struct is aligned. For example here I set some values for the psr register but when printing it directly it did not show up.

Maybe a we could create a union for each register separately, something like

typedef union {
    union {
        struct {
			uint32_t TEC   : 8;
			uint32_t REC   : 7;
			uint32_t RP    : 1;
			uint32_t CEL   : 8;
			uint32_t _res  : 8;  // Padding to fill 32 bits
		} bits;
        uint32_t raw;
    } ecr;

    union{
        struct {
			uint32_t LEC   : 3;
			uint32_t ACT   : 2;
			uint32_t EP    : 1;
			uint32_t EW    : 1;
			uint32_t BO    : 1;
			uint32_t DLEC  : 3;
			uint32_t RESI  : 1;
			uint32_t RBRS  : 1;
			uint32_t REDL  : 1;
			uint32_t PXE   : 1;
			uint32_t RES   : 1;
			uint32_t TDCV  : 7;
			uint32_t _res  : 9;  // Padding to fill 32 bits
		} bits;
        uint32_t raw;
    } psr;
} FDCAN_StatusRegisters_t;

It would then however be 8 bytes in size in any case. Maybe a better solution would be to just add __attribute__((__packed__)) to the structs? Then FDCAN_StatusRegisters_t would be 6 bytes and as far as I can tell the alignment would match.

[raffael0]

Good point. I'm not very experienced in unions in c.

Another problem I've noticed is the ordering of the bitfield. Apparently the C Specification defines no ordering so it could be left-to-right or right-to-left depending on compiler and platform. This rules kind of rules out using the feature for our usecase. We don't really know if our cpp compiler for the llserver reads it the same way that the firmware code sends it... fucking c

Instead I could do it like the HAL from stm we are using and only define positions and lengths and do the actual inserting/reading with bit manipulation. Something like this:

#DEFINE LEC_POS 0
#DEFINE LEC_LEN 3

#DEFINE READ_BITS(REG, POS, LEN) (REG+POS) &=((1<<LEN)-1)
#DEFINE SET_BITS(REG, POS,LEN, VAL) (REG+POS) = (VAL & (1<<LEN)-1)

I really hate this idea because it makes the code so much more complicated.
Any ideas on how to fix this?

EDIT:
I looked into it a bit more and found this blog post which looks promising

[miDeb]

That blog post was interesting, but it only allows bit fields of the size of 8, 16, 32 and 64. I also found this post with a similar but different implementation, but I found it a bit unreadable to be honest. I attempted to implement something similar, which you can find here. I'm not sure we want to use that code because despite my best attempts it is still quite unreadable. The performance impact of packing and unpacking the bits is probably also quite heavy, as opposed to using simple macros. On the other hand defining bitfields would be straightforward and clean. So idk what the best way forward would be.