search

TrickfireRobotics / mission-control

The mission control user interface for "Viator" for the University Rover Challenge
1 stars 2 forks source link

[Adamc/restructure-cleanup-codebase] Refactored entire codebase #28

Closed adamseth2 closed 2 weeks ago

adamseth2 commented 3 weeks ago

List of Changes

11/8/2024

Structure of Codebase

Questions

  1. Right now, the vue components directly use const rosjs = useRoslibStore(); to access their generic pub and sub. Should .vue use this or should, for example, Controller.vue solely rely on the controllerStore to interact with ros? This would add a lot more boilerplate, but potentially reduces decoupling and perhaps fewer errors as you have to explicitly create functions that serve one purpose.
  2. Any other glaring issues?
loukylor commented 3 weeks ago

As for your question Adam, I don't think relying on more stores is really necessary. I'm not super familiar with the state management system you've added, but just from my gut feeling, there doesn't need to be a store. Vue objects are already decently stateful, and global states are needed when many components pull from the same data source or rely on each other's state. Since there isn't a ton of that in the individual components, I don't think it's needed. At some point in the future, there may be a necessity for them, but that isn't now.

adamseth2 commented 3 weeks ago

@uellenberg The camera code will be shipped later as the rover code on the camera side completely changed. And as the camera code here is outdated already, thought it doesn't make sense to still have it. Will look into whether patch is needed still. I tried inserting the type into Topic but there was a problem with undefined, but I haven't looked into it too much.

adamseth2 commented 3 weeks ago

Okay, I now remember why I did the whole patch thing last year and did not pass into Topic. Unless I am missing a step, the problem is with passing the type into ROSLIB.Topic is that the response would expect T. This would work though but I don't know why Roslib is coded this way but the subscribe callback actually ALWAYS returns the json Type {data : unknown} not T (I've done extensive testing and experimenting with the rover).

This code is from me playing in the master branch, but this is what typescript would yell at you for:

Passing type into ROSLIB.Topic: image

Passing the exact json type object that could remedy this problem but also this is a base case feature that every topic should have: image

Okay. I've thought about it again and I'll remove the patch because long term it is a bad solution when updating dependencies and that could potentially break the codebase. I'll just make it a type, and make it generic. Side effect it would have to be something like the 2nd image of passing in a wrapper Type (it would be generic just too lazy to update the screenshot)

uellenberg commented 3 weeks ago

Gotcha, that makes sense. It looks like Topic takes in the type of the entire message, including the data field. Presumably there are messages without a data field, but I'm not sure what they are. The easiest solution would probably making an:

export interface Msg<T> {
    data: T,
}

Somewhere, then writing Topic<Msg<number>>. That's a bit annoying though, but I'm not sure if data can ever not be there, so I'm hesitant to modify the types to always include a data field.

loukylor commented 2 weeks ago

If you look at the typing for roslibjs, it shows that the parameter in the callback of subscribe is of type TMessage, which is the type parameter that you pass into topic. So, in the code that is erroring, typescript thinks that message has the type number. It doesn't have a .data attribute, so of course it errors.

As how to actually fix the code, you have two options. Again, if you look at the typing again, notice that TMessage = Message, i.e. it when no type parameter is passed, TMessage will equal Message by default. Message has no attributes, so we probably shoudn't use it, but this is option one.

Option two, which is probably what we should do, is to pass in our own types into topic. Standard messages have a data field, so you can make a generic StdMsg<T> type, but all others that we define will need their own type and may not have a data field. These types that we define and aren't standard messages will need to be coordinated between the rover teams to ensure we're interpreting the data correctly.

adamseth2 commented 2 weeks ago

Yeah, I have implemented option 2. Ohhh I never fully realized that is why data was always there (2 images below shows both containing data. Thanks for pointing that out.

I'll keep note of it, moving on forward. I was debating if I should pull out the entire Typescript power and all the conditional typing stuff, but right now it's like 20x easier if there is a custom type, they ensure they have .data attribute and that's only if the mission control needs it anyway. The other stuff attributes like header etc like in CompressedImage don't really matter as we should only care about the actual data. (though the way it is coded right now (haven't pushed it yet) it could be implemented in the future not too hard just not a huge priority as we aren't using much custom types )

image

image

adamseth2 commented 2 weeks ago

Ready for another look. I believe it is basically merge-ready now.

loukylor commented 2 weeks ago

just a couple small changes are needed

adamseth2 commented 2 weeks ago

Applied all feedback

loukylor commented 2 weeks ago

Sounds good, I'm quite busy as of now and won't be able to really deep dive for another few days. Since this is time sensitive waiting that long isn't too viable. I trust that you've applied all my feedback. Thanks Jonah for reviewing and Adam for doing all this. Good work everyone!

adamseth2 commented 2 weeks ago

No worries Kyler! I thought you were busy with other stuff and you already did so much. Again thanks to both of you for getting me many rounds of iterative feedback they were extremely helpful. Hopefully, this is the last BIG PR, and the rest would be very small. Again thanks!