[Rust] API design and programming practices

[Thom-de-Jong]

Hello everyone,

While implementing the core concepts of the AgIsoStack in Rust, I came across some problems trying to translate the C++ API 1 to 1.
Of course I expected to find some minor inconveniences as Rust requires us to think more about the data and flow here of.
I don't see a problem with "Rustifing" the API, e.g. changing function names from get_name() to just name(). This will just make Rust programmers more at home without changing a lot about the library.

The main problems I have encountered fall into 3 categories,

• Global statics (e.g. Singleton manager classes)
• Multi threading build into the library
• Callbacks

I would like to discuss with you and express my Rust based opinion on these topics.

Global statics

Based mostly on can_internal_control_function.cpp
This class contains a list called internalControlFunctionList which is a list with pointers to all existing control functions.
Now, I get the point of doing it this way in C++ as it gives the CANNetworkManager easy access to all the control functions.
In Rust however, this will give us problems with the Borrow checker and changing values contained in statics is always considered "Unsafe".

This can be fixed by just not having the static list of references and having the owner of the control function call update() manually. Passing in any network manager or data, etc. as needed.
(The owner could be; the end user application, a virtual terminal client, task controller client, etc.)

This will however change the public API quite significantly.

Multi-threading

Building multi-threading into a library in Rust, at least in the embedded (no_std) world, is not possible, as a lot of embedded targets do not support the standard library.
To make the library available to as much platforms as possible, I would separate the library into layers, in the same way as it is done now, but let the end user decide how to process each layer. (One after another, using std::threads, using an embedded schedular like embassy, etc.)
Each layer should thus never block and be completely seperated from the others. Communication between layers is normaly handeled by the user application. Each layer only proccesses the data it is given.

This will again change the public API quite significantly.

Callbacks

Callbacks given to TestVirtualTerminalClient->add_vt_soft_key_event_listener() are not easily possible in Rust. The simple reason is that callbacks can not change the global state without access to global statics, which again, is "unsafe".
I don't like forcing the end user to write "unsafe" Rust.
I have solved this problem before by having something like TestVirtualTerminalClient.next_event() and polling the VT client in the user application.

And again this will change the API.

To finish of...

To finish of my rambling, I would like your opinions on the following thought;
"The library should only do work when it is explicitly asked to."
This is a concept most Rust libraries stick to. (as does the core rust team)

I hope we could have a nice discussion about these programming practices.

[GwnDaan]

Hey there!

Great to have you onboard for the Rust implementation of the stack! Note that the C++ implementation still is far from stable, and therefore a lot of the API is likely to change until we have a stable version. Hence translating the C++ API 1:1 to Rust might still be a tad to early if you ask me. #161 being the biggest core change that will occur probably.

Global statics

I agree with you here up to some point. Static globals are convenient to have in some places, but we should try to not abuse them. As the C++ google style guide states (read the details here):

Global and static variables that use dynamic initialization or have non-trivial destructors create complexity that can easily lead to hard-to-find bugs.

This is also the case for the internalControlFunctionList as it's internal addressClaimingStateMachine member has a non-trivial destructor. Hence refactoring away from it is in my opinion the right choice. Not only for the Rust implementation but also on the C++ side. #282 proposes exactly this.

The same currently holds in other places as well, e.g. the DiagnosticsProtocol and the PGNRequestProtocol and probably more.

Where I don't totally agree is by letting the user update() the internalControlFunction themselves. As @ad3154 said before in #161:

I really wanted to completely abstract that away from the user so it was impossible to mess up, fully automatic, and completely required to communicate on the bus. Currently if they make an ICF, we implicitly know they want to talk on the bus, so it automatically claims and manages its own address. If the bus is not in a usable state, it just keeps trying until it is in a usable state, with no function calls required by the user outside of constructing an ICF. Internally we take all control of this, besides preferred address, away from the user.

A better approach than manually calling update on the InternalCF's might be to have a reference list of all InternalControlFunctions in the CANNetworkManager class itself. The CANManager.update() function can then call InternalControlFunction.update() for all known ICF's. (Again see #282 for proposal)

Multi-threading

I agree, using std::threading initially was a great approach to how we handle several layers. But embedded systems don't always seem to support it. For example the Arduino IDE (#264) requires us to also refactor away from std::thread to support all of it's compatible boards. In theory removing std::thread doesn't have to lead to a major overhaul of the C++ stack. In most places we can simply make the relevant update() functions public and have the application call them themselves. The only downside is that their application sometimes might "not work" according to users when they forget to call the update(). But on the other hand it improves "The library should only do work when it is explicitly asked to" experience. I'm not sure on this one. Maybe we can come up with a nice middle ground?

Callbacks

I don't like the idea of polling for events. If there's no other way, sure. Or if it's really beneficial for performance, I'm okay with it. But there seems to several ways to the observer pattern in Rust: stackoverflow. Note how we also use the observer pattern with weak references in the C++ implementation: event_dispatcher.hpp.

Sorry for the wall of text haha, just writing my thoughts down. Love to hear opinions on all this 😄

[Thom-de-Jong]

Hi Daan

Thanks for the comprehensive response!
I have read #161 and #282 and think those are great changes.

Global statics

Great change, having multiple CANNetworkManager's, each responsible for there own InternalControlFunction's is something that will work in Rust.
Using statics is a difficult subject in Rust. Internally I am ok with it, as long as the end user does not have to interact with them, as this forces the use of unsafe{} Rust.
We shall see what we can come up with 😄

Multi-threading

"The library should only do work when it is explicitly asked to"
Calling one update() function in a loop would satisfy this if you ask me.
This way it is clear for everyone that we, the end user, want the stack to do its magic.
For example in (examples/virtual_terminal/version3_object_pool/main.cpp)
Instead of this;

while (running)
{
	// CAN stack runs in other threads. Do nothing forever.
	std::this_thread::sleep_for(std::chrono::milliseconds(1000));
}

I'm in favor of this;

while (running)
{
	// Explicitly call the update method in the loop.
	VirtualTerminalClient->update();
}

Callbacks

In C++ I would also never poll for events, because we were probably both taught that in school. 😝
Rust however is a bit different because callbacks require circular references.
(Let me know if I am using to many Rust specific thoughts/ concepts.)
This can be implemented by using "weak", reference counted pointers with interior mutability.

One problem is that, on embedded systems, Rust does not yet support reference counted pointers...
It does support interior mutability so maybe I can work around this problem.
I will do some more research to see if it can be done on embedded Rust.

Callbacks given to TestVirtualTerminalClient->add_vt_soft_key_event_listener() are not easily possible in Rust. The simple reason is that callbacks can not change the global state without access to global statics, which again, is "unsafe".

There is a simple workaround for the problem above so I can ignore that concern for now.