Remove Incoming Message Pipeline, Modularize CAN Utilities

[ArnavG-it]

Currently, the CAN Wrapper is the first to be notified of a received message (through the HAL_CAN_RxFifo0MsgPendingCallback ISR), and the message is queued until the user calls CANWrapper_Poll_Events to process it. This pipeline complicates TUK and limits flexibility for the subsystems and test interfaces, especially due to the various CAN queue implementations.

This pipeline should be removed from TUK. Instead, subsystems should implement their own queues and ISRs.

The CAN Wrapper can still handle peripheral initialization, transmission, timeouts, acknowledgement, etc., but these should be made into modular functions where possible.

[ArnavG-it]

@Koloss0 Let me know if the issue corresponds with what you were thinking. I think you have the ability to edit it if you want.

I do have one idea/question before I start deleting stuff. We could keep the HAL ISR in the wrapper, but just remove the queue stuff and have it trigger a user callback. The callback would have to be treated like an ISR, so it's not like message_callback (which would just be removed). Subsystems would get the CANMessage after it's been deserialized (and the CAN handle) and then choose how they want to queue it. I think the main benefit would just be hiding the HAL stuff, and also saving an extra deserialization function call in every subsystem's ISR. If you prefer just handing the entire ISR over to the subsystems (or see a problem with this approach), I'll continue with that.

[Koloss0]

@ArnavG-it Couldn't have said it better myself. That's exactly what I meant. "Modular" can mean a lot of things but I know what you're getting at. The word might be stand-alone functions.

As for your idea, that's a fantastic idea. That would actually be just like how the Immediate Mode used to work, when that existed. The funny thing is we actually removed that because we thought it complicated CDH as they had to do their own queueing/de-queuing. So we're kinda going full circle, but wiser. I think we can still keep the name message_callback, since it's still a callback for incoming messages. But the expectation would be that it's just used to enqueue messages (probably just one line).

[ArnavG-it]

@Koloss0 I think in terms of usability, this is almost done. I'm mostly just deleting stuff, after all. I started on integrating TUK into the testbed, mainly to make sure it compiles (which it does). Oh, and the wiki will need updates obviously. A few small decisions to make, though:

1. CANWrapper_Poll_Events doesn't make sense anymore since the queue won't be tracked internally, so the two underlying functions (CANWrapper_Process_Message and CANWrapper_Poll_Errors) would have to be exposed to the user. I'm guessing the reason for hiding the error function is just so that ack'ed messages can be deleted from the Tx cache before being falsely reported as timeouts? I can't think of a way to enforce the same thing with this refactor, but we could just trust the user to do it in the right order. Let me know if you have any thoughts on that.

2. Do you think the processing function should be split up? The Tx cache part could go in the ISR, or be its own function if that's too much overhead. If the acknowledgement is its own function, we could get rid of manual mode entirely. We could also just leave the function alone (but maybe name it to Handle_Ack or something).

void CANWrapper_Process_Message(CANMessage *msg)
{
	if (msg->is_ack)
	{
		// Search the TxCache to verify that this ACK corresponds to
		// something we sent. Delete the entry if it is.
		int index = TxCache_Find(&s_tx_cache, msg);
		TxCache_Erase(&s_tx_cache, index);
	}
#ifndef CWM_MODE_MANUAL
	else
	{
		// Acknowledge the received message.
		transmit_internal(msg->sender, msg->cmd, msg->body, true);
	}
#endif
}

3. With the internal queue removed, RTOS mode is only referenced by the message task. Removing it would remove the "modes" for good. Up to you.

4. Confirm for me whether you want transmit_internal to be separated into stand-alone functions? As far as I can tell, it's fine as is, but if you want to go for the "toolbox" approach I could split it up into serialization, delaying, etc.

5. can_wrapper.h includes some STM32L4-specific drivers. The IO board needs the STM32F7 equivalent of the same files. It's just a matter of changing those two letters in each file, but let me know how you want to go about that.

#include <stm32l4xx.h> -> #include <stm32f7xx.h>
#include <stm32l4xx_hal_def.h> -> ... etc.
#include <stm32l4xx_hal_can.h>
#include <stm32l4xx_hal_tim.h>

[Koloss0]

@ArnavG-it These a great questions, I had to think about it for a bit.

I think you're basically asking if it should look something like this (user-side):

// Process message queue.
CANMessage msg;
while (CANQueue_Dequeue(&s_msg_queue, &msg) == ERR_OK)
{
	if (msg->is_ack) {
		CANWrapper_Process_Ack(&msg);
	} else {
		CANWrapper_Transmit_Ack(&msg);
	}
}

And yeah, I think that's not bad. We can even create a CANWrapper_Process_Queue() function which does all of this for you, and calls a callback:

CANWrapper_Process_Queue(&my_queue, &my_callback);

Then for the more niche cases, (like SOTI and the IO board) they write their own logic instead of calling the pre-defined routine.

Now to answer your points directly:

1. Your assumption is correct. Combining them was a simple way to enforce the correct ordering. But it's fine not to, it's all tradeoffs.
2. Yes, that's a good idea. A little extra code on the user side to eliminate the #ifdefs isn't a bad trade-off. The user can decide for themselves if they want to transmit an ACK in their queue processing logic.
3. We can rename the symbol to something more appropriate, but we wouldn't want to get rid of it entirely since that file includes cmsis_os.h which I'm pretty sure is an RTOS file.
4. I think keeping it as-is is fine.
5. Good point. The best solution I can think of is to replace all the HAL includes with #include <hal_include.h>. Then create a file something like this:

// hal_include.h

#if defined(STM32L4)
#include "stm32l4xx.h"
#include "stm32l4xx_hal_def.h"
#include "stm32l4xx_hal_can.h"
#include "stm32l4xx_hal_tim.h"

#elif defined(STM32F7)
#include "stm32f7xx.h"
#include "stm32f7xx_hal_def.h"
#include "stm32f7xx_hal_can.h"
#include "stm32f7xx_hal_tim.h"

#else
#error "Unsupported STM32 family"
#endif

It's not pretty, but IMO it's better than the alternatives.

Looking at the compiler symbols in the project settings, it seems like STM32CubeIDE might even pre-define STM32L452xx for L452's. So we could use that if we want. But, I'm gonna say let's just define our own STM32L4 symbol. We'll have to update the TUK installation instructions to include a step to define it.

[Koloss0]

@ArnavG-it Some new developments: I was just talking with Daigh about possibly adding RTOS to the rest of the subsystems. The sentiment I often hear from professionals is that writing software on bare metal is generally not advisable in most cases. I need to look into the exact rationale for this, but if we do it, it could significantly change how we approach this redesign.

[Koloss0]

Like all great software projects, we completely changed course on this issue. This entire architectural problem gets resolved as soon as we force RTOS on all the subsystems. Case closed forever... I hope.