ROSplane code clean up, reorganization, and refactoring

[bsutherland333]

This includes getting rid of dead code, updating license statements, renaming "example" and "base" classes to something more meaningful and dividing packages more sensibly (does data_viz and rosplane_sim need to exist?).

[JMoore5353]

I'm not sure data_viz needs to exist. It seems like the performance is much worse than Plotjuggler, and it is much less capable/versatile than Plotjuggler. It essentially just eats up CPU in my workflow. Should we get rid of it? What are your thoughts @iandareid @bsutherland333 ?

[bsutherland333]

I think this was Ian's plan to do at some point. Ideally we'd have the plots we need in the ground station RQT gui and could then use plot juggler when we really need to dig into the performance.

[JMoore5353]

We are merging #9 with this issue

Repeating description from other issue: There is a style guide on the ROSflight website that is used to help keep all code formatted consistently and cleanly. Since this was an external repo that we are now migrating into the ROSflight ecosystem, some of the code needs to be reformatted to follow the style guide.

We could also copy over the .clang-format file, script, and workflow from the rosflight_ros_packages or rosflight_firmware repositories to help with this.

[iandareid]

Repeating description from other issue: There is a style guide on the ROSflight website that is used to help keep all code formatted consistently and cleanly. Since this was an external repo that we are now migrating into the ROSflight ecosystem, some of the code needs to be reformatted to follow the style guide.

We could also copy over the .clang-format file, script, and workflow from the rosflight_ros_packages or rosflight_firmware repositories to help with this.

I think we need to have a discussion on the style guide before we make changes to ROSplane. Reading through it, looks like ROSplane was not developed with this sytle guide, some things are only partially implemented, or just not at all. We need to decide what we want to keep and what we want to add, then the GREAT REFACTORING can begin.

[iandareid]

There are also some structure issues I think that exist in the other parts of ROSplane.

The files at the very least need to be renamed from xxxx_example.hpp/cpp. Also when it came to implementing my own controller for ROSplane it yielded dividends in structural changes. I am worried that there similar breaking changes that will occur if we sat down and thought about how we would implement a new path_manager or path_follower and even path_planner. I would argue that we need to sit down before the v2.0 and just double check there would be no breaking changes. I would hate to release 2.0 and realize just a few months later that a 3.0 is required for splines or RRT or something.

[JMoore5353]

Another thought to consider: we currently don't use the set_current field in the rosplane_msgs::msg::Waypoint objects... Should we get rid of it?

[iandareid]

Perhaps we should add a set_current as a service in the path_planner?

That way if you wanted you could have pilot manually fly it to a region of interest and then set the waypoint there. This would be particularly useful for JJ's research.

[JMoore5353]

Refactoring change: In the controller class, there are a couple structs used to define inputs and outputs. Should we change this to use just the message objects that they are sending? This would remove duplicate code between the message definition and the structs in the code.

Thoughts?

[bsutherland333]

The intent of this may have been to keep ROS out of the example classes. @iandareid would know better.

[iandareid]

I have been bothered by the structs for a while, they are present in most of the nodes. They lock in functionality into the estimator_base that I think shouldn't be there. I have thought of removing them as being passed in and making them member variables. I like the idea of making them the same as the message type, but unless I am wrong, I think occasionally there is more data in the structs than the messages. I am not sure. This would be change to the style guide however, they insist that returned objects, or mutated objects should be passed by reference. In any case, a change should be made that would not require someone to modify a random struct in estimator_base to reflect changes in estimator_example.

[JMoore5353]

The intent of this may have been to keep ROS out of the example classes. @iandareid would know better.

That makes sense. However, having structs with the same data as a waypoint message seems worse than having the estimator_example have access to the ROS message definition. If something were to change, for example, then you'd have to change the definitions in two spots. I think the purpose of the base class is fulfilled if it manages all of the ROS2 interfaces (publishers / subscribers / etc.), but it doesn't have to contain everything ROS-related (e.g., a message object definition).

Having those objects be the message object types also adds some cohesion to what the input_ and output_ variables are, since it is a consistent object type being passed around.

I like the idea of making them the same as the message type, but unless I am wrong, I think occasionally there is more data in the structs than the messages.

If there is, then we should consider if it is important enough to add to the message definition. If not, then we could probably get rid of it or do something different. In most cases, it seems like those structs are primarily used to pass information from the derived classes back up to the base class, which immediately packages them into the message definition and publishes it.

This would be change to the style guide however, they insist that returned objects, or mutated objects should be passed by reference.

We could also still pass in the variables by reference. It would seem reasonable and clear in the code that the base classes would expect to pass in the request (subscribed message type) objects and have the example classes fill in the result (published message type) objects. I guess we could pass the result object by reference or return the result object, but I would probably prefer that over creating member variables.

Thoughts?

[iandareid]

If something were to change, for example, then you'd have to change the definitions in two spots.

I would like to avoid this so you make a compelling point.

If not, then we could probably get rid of it or do something different

I think these sorts of things could be member variables, but we should look at an example of what one of these entries even is.

I would probably prefer that over creating member variables.

We save the messages of subscriptions as they come in as message objects as member variables for reference in the loops that take in input and output structs, so I may actually prefer just accessing those, which may be even more clear where those values come from, and if we are already doing that why not just have a most up-to-date message that we update and send each call but hold onto between calls as a member variable. It could be convenient when we need to have the state from the directly previous iteration.

Should we duel for who's preference is right?