-
-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Core]: Detached message handling #462
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it certainly is more abstraction, as we'd now be abstracting the entire network manager from the higher level interfaces. This could be interesting for testing I suppose. It maybe has some weird implications in the app, where now you can't store some interfaces on the stack, you have to make shared_ptr's if you want to really use them, which IDK if is amazing, but I also understand... Trying to think of how someone could, in the future, maybe put a network manager on their local stack, plus all their various interfaces, seems like we'd need references or raw pointers, which I also don't love... so maybe all the shared_ptrs is fine. We've been going that route for some time now.
Adding a vtable to each of these interfaces is the same size as a reference or something to the network manager, so that's nice - for about the same class size you can abstract more stuff away.
I guess what I'm saying is I don't dislike it. I think it will get the job done. I think we do keep getting further from the ability to ever really make the stack static allocation friendly, which is maybe fine? I know some levels of functional safety forbid any dynamic memory - which would be pretty painful for us to deal with, so maybe it's not worth worrying about especially with nobody explicitly asking for it or sponsoring that kind of thing?
So I suppose I'm interested in your thoughts in return, but I think it's a fine approach.
Yeah I know... Even though it would have been super nice to also be able to support those kind of strict rule sets, I think static allocation is out of our scope at the moment. Especially since we have always worked with the dynamic allocation and expanded on that. The shared pointers already help a lot to manage the lifecycle of those dynamically allocated objects. Converting the whole stack to be static allocation friendly I imagine will complicate so many things, it would be almost a rewrite... Edit: hmm, it might be doable with some clever bit of macro usage to replace all shared pointers with raw pointers, similar to what we do with the threaded stuff, but it will still definitely complicate things. I don't think this PR will make it any harder than it was before, instead it might even make it easier as we abstract things, it can be easier to (progressively) refactor things later on. |
3fc63a7
to
e55177e
Compare
Yeah... but I do think we should worry about our current stuff and releasing 1.0 before we think too hard about it the static allocation stuff I think (if ever) I was mostly just thinking out loud 😊 |
and temporarily convert the messaging provider to a raw pointer until the network manager is no longer a singleton
e636772
to
bccc405
Compare
|
9fbb934
to
13593af
Compare
Describe your changes
Introduced 3 classes that will remove the the dependency on the CANNetworkManager for all classes that want to receive / send messages. namely:
I think this will be a better alternative than passing the network manager around everywhere when we are refactoring away from the singleton. And it might especially be useful for unit testing, where we can isolate a class we wish to test and thereby remove interference from others. Though I'd like to hear any thoughts on this, as it does surely add an extra layer off complexity to the stack.
I see 3 ways a consumer can be set-up;
CANMessageHandler
to the consumer class and let it register itself, i.e.:I think I prefer the last one. To start off, I switched over the ShortcutButtonInterface and HeartbeatInterface to this messaging system so we can see what it's like.
What's next
Once we find and agree on an approach, all the other classes that send/receive messages have to be adopted to this method.
Furthermore, I'd like some unit-tests that will be run first in line to make sure the system remains correctly in place and it doesn't cause other tests to fail.