clean up data structures, remove dynamic memory allocation and make use of Linux compatible lists

[marckleinebudde]

This patch series cleans up the existing data structures and removed dynamic memory allocation entirely from the code. Last but not least the queue implementation is replaced by a Linux compatible list implementation.

On a STM32F072 the max TX CAN-Bus load (1 Mbit/s, DLC=1) increased from 77% to 87%. A 100% loaded bus can RX'ed without problems before and after the change.

[ryedwards]

Can we also look into changing can_data_t to CAN_HandleTypeDef? This allows easier access to the per channel registers and allows moves the init values into a more standard structure.

[marckleinebudde]

I think the code doesn't make use of CAN_HandleTypeDef, yet? Where do you want to use it? Can you illustrate with a patch on top of this branch?

[ryedwards]

Now that I reviewed it, it makes more sense for it to roll with the CANFD stuff.

[fenugrec]

I'm slowly reviewing this now.

Question 1 : at this moment I don't see why you move USBD_GS_CAN_HandleTypeDef to a header :

• we're only calling USBD_GS_CAN_Init once, so we only ever need one "instance" of that struct ? I.e. why not just static USBD_GS_CAN_HandleTypeDef hcan = {0} in usbd_gs_can.c
• we lose (maybe on purpose... haven't gone through all the commits here yet) some data hiding - I usually prefer to keep structs like this private

[marckleinebudde]

Question 1 : at this moment I don't see why you move USBD_GS_CAN_HandleTypeDef to a header :

USBD_GS_CAN_HandleTypeDef is becoming the structure describing the whole device.
In the 2nd patch I put it into the BSS in main.c, one less calloc().

• we're only calling USBD_GS_CAN_Init once, so we only ever need one "instance" of that struct ?

ACK, we need only 1 instance of USBD_GS_CAN_HandleTypeDef.

I.e. why not just static USBD_GS_CAN_HandleTypeDef hcan = {0} in usbd_gs_can.c

I put the queues/lists and CAN channels into that struct, so it's needed in a header and it can't be static. Doesn't really matter in which file lives...

• we lose (maybe on purpose... haven't gone through all the commits here yet) some data hiding - I usually prefer to keep structs like this private

It's one big driver anyway, isn't it :) For me it was strange to have so many pointers to stuff that was global anyway.

[fenugrec]

I put the queues/lists and CAN channels into that struct, so it's needed in a header

Fair, I hadn't made it that far.
But then you added USBD_GS_CAN_get_channel() - if we're going for a global struct, why not just hcan->channels[x] since the channel number validation happens elsewhere ?

[marckleinebudde]

USBD_GS_CAN_get_channel() does the channel evaluation! It returns NULL or a pointer to a valid channel. So far there was no validation of the channel number. 😸

But USBD_GS_CAN_get_channel() is added in my multichannel branch, which is still WIP marckleinebudde@6ae71a2

[fenugrec]

So far there was no validation of the channel number

Right.

my multichannel branch, which is still WIP

Ah, I was just scrolling through gitk and didn't notice it was a separate branch.

Other topic :

#define hlist_for_each_safe(p, n, head)                 \
	for (p = (head)->first; p && ({ n = (p)->next; 1; }); p = n)

Heh, that (and other uses of ({....}) had me scratching my head for a while. Had to look it up :

https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

Alright, this should be good to go. Do you want me to rebase or will you do it ?

[marckleinebudde]

Rebased. One more thing, @ryedwards says debug build are not working anymore. I haven't tried to reproduce the issue here. Maybe debug stuff needs heap? I've increased it from 0 to 512. Still works on my STM32F072. Can someone with a 042 test?

[fenugrec]

@ryedwards says debug build are not working anymore.

Oh, I missed that - where ?
we don't explicitly have a Debug config in our Cmakelists , and compiling with just -g (i.e. -O0) probably wouldn't ever fit on an F0 ...

Can someone with a 042 test?

works on 3e33589 (current master), and on replace-queues@fa1b0dfa5307151f1c748a029cd05d52d0a21e33 it's broken - not even enumerating. Investigating now.

[marckleinebudde]

@ryedwards says debug build are not working anymore.

Oh, I missed that - where ?

There's an IRC channel #socketcan on https://web.libera.chat/

[marckleinebudde]

works on 3e33589 (current master), and on replace-queues@fa1b0dfa5307151f1c748a029cd05d52d0a21e33 it's broken - not even enumerating. Investigating now.

😢

[ryedwards]

works on 3e33589 (current master), and on replace-queues@fa1b0dfa5307151f1c748a029cd05d52d0a21e33 it's broken - not even enumerating. Investigating now.

In debug mode it would hit the hard fault IRQ even before the debugger had a chance to halt at main(). I was going to try and back out changes and see which commit broke it but I haven't had a chance. Once I get my other stuff merged I'll dive into it.

[marckleinebudde]

The branch should be bisectable.

[fenugrec]

I think I have an idea of the issue causing the hardfault. in startup.c :

typedef struct _zero_table_t
{
	uint32_t* dest;
	uint32_t wlen;
} zero_table_t;


void Reset_Handler(void)
{
....

	for (zero_table_t const* table = &__zero_table_start__; table < &__zero_table_end__; ++table)
	{
		for (size_t i=0; i<table->wlen; ++i)
		{
			table->dest[i] = 0u;
		}
	}

the for loop runs wlen times, but it writes a u32 every loop. Perfect, but the ldscripts calculate wlen with LONG (__data_end__ - __data_start__) , i.e. a value in bytes !

So the hardfault occurs on a build here with wlen=0x930 , at i=0x601 (thus trying to write at 0x2000 1804 - right outside an F042 's 6kB of RAM !)

[marckleinebudde]

I think I have an idea of the issue causing the hardfault. in startup.c :

typedef struct _zero_table_t
{
	uint32_t* dest;
	uint32_t wlen;
} zero_table_t;


void Reset_Handler(void)
{
....

	for (zero_table_t const* table = &__zero_table_start__; table < &__zero_table_end__; ++table)
	{
		for (size_t i=0; i<table->wlen; ++i)
		{
			table->dest[i] = 0u;
		}
	}

the for loop runs wlen times, but it writes a u32 every loop. Perfect, but the ldscripts calculate wlen with LONG (__data_end__ - __data_start__) , i.e. a value in bytes !

__data_ is used in the __copy_table, not the __zero_table.

So the hardfault occurs on a build here with wlen=0x930 , at i=0x601 (thus trying to write at 0x2000 1804 - right outside an F042 's 6kB of RAM !)

With my setup there is no __zero_table in the ELF file, just a __copy_table

.copy.table     0x00000000080037b0        0xc
                0x00000000080037b0                . = ALIGN (0x4)
                0x00000000080037b0                __copy_table_start__ = .
                0x00000000080037b0        0x4 LONG 0x80037bc __etext
                0x00000000080037b4        0x4 LONG 0x20000000 __data_start__
                0x00000000080037b8        0x4 LONG 0x964 (__data_end__ - __data_start__)
                0x00000000080037bc                __copy_table_end__ = .

.zero.table     0x00000000080037bc        0x0
                0x00000000080037bc                . = ALIGN (0x4)
                0x00000000080037bc                __zero_table_start__ = .
                0x00000000080037bc                __zero_table_end__ = .
                0x00000000080037bc                __etext = ALIGN (0x4)

Map file generated with this hack (works only if you compile a single firmware):

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 77514a2fbb07..8608732b1290 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -38,6 +38,7 @@ add_link_options(
        -mthumb
        LINKER:--gc-sections
        LINKER:--print-memory-usage
+       LINKER:-Map=foo.map
 )
 
 add_subdirectory(libs/STM32_HAL)

[marckleinebudde]

Changes:

• set heap for STMF072 and STMF042 to 0 bytes (again)
• don't statically initialize the struct list_head in hGS_CAN, but during runtime. This way hGS_CAN will end up in the BSS and not in the data section. This probably works around the issue on STM32F042 (startup: get rid of zero table and fix copy_table #141).

[marckleinebudde]

rebased to new master

[fenugrec]

I'm almost done merging -
interestingly, I'm not seeing the same improvements on bus load here : I was already getting 99%+ bus loads (with cangen -g 0 -p 10 can5 + canbusload can0@500000 -e) before.

[marckleinebudde]

changes:

• use {...} for single line if statements

[marckleinebudde]

canfdtest -vg can0 at 1MBit/s fails after some minutes. Will test with current master.

[marckleinebudde]

changes:

• try to exactly replicate what the queue implementation does

[marckleinebudde]

changes:

• factor out conversion send_to_host_or_enqueue() to queue_push_back() into separate patch
• main(): CAN bus errors: replace list_add_locked() by list_add_tail_locked() for hGS_CAN.list_frame_pool

[fenugrec]

[edit] disregard, was on wrong commit

[marckleinebudde]

rebased to latest master

[fenugrec]

I ran canfdtest and cansequence for about 1h each in both directions. All seems well here. Everything still good at your end ?

I'll probably tag a release on the commit just before the -O2 / -Os change, as we're adding more serious changes

[marckleinebudde]

Sounds like a plan!

[fenugrec]

Merged. Thanks for your diligent rebasing !