byu-magicc / rosflight_plugins

Container for basic sensors needed for Gazebo simulation.
8 stars 9 forks source link

Rename this package to ROSmav_plugins? #6

Open wynn4 opened 7 years ago

wynn4 commented 7 years ago

Does anyone else agree? What were the reasons for going with rosflight_plugins? It just seems like these plugins are more for ROSplane and ROScopter and should thus be called ROSmav_plugins. Maybe I'm wrong.

plusk01 commented 7 years ago

Yeah, this was brought up. I think the reason we ended us going with rosflight_plugins was because these plugins are grouped with the ROSflight stack. I see arguments both ways...

wynn4 commented 7 years ago

OK that makes sense. I can see that now. It still seems like it's not quite right to me, but it makes sense

dpkoch commented 7 years ago

I agree with @wynn4... yeah I guess they're kind of maybe loosely grouped with the ROSflight stack? But really it seems like they're grouped with the ROSplane/ROScopter stacks to me... hence ROSmav

plusk01 commented 7 years ago

I would support this. @gellings @jerelbn @superjax do any of you oppose?

gellings commented 7 years ago

Either way, I like rosflight_plugins but I don't care very much.

jerelbn commented 7 years ago

ROSmav makes more sense to me.

mmmfarrell commented 4 years ago

@BillThePlatypus We should make this change.

In addition, I think it may be best if this repository was a sub module in both roscopter_sim and rosplane_sim. It just isn't super clean that roscopter_sim and rosplane_sim depend on this repository, but not a specific branch or commit.

What if I push changes up to this repository that unknowingly breaks something in rosplane_sim? Then someone will try to clone the master branch of this repo and build and run it with the master branch of rosplane_sim and it won't work.

Better if rosplane_sim and roscopter_sim both depended on specific commits (via sub module) so that is not a possibility.

@gellings @dpkoch (The only people from this string left in the lab) Any opinions?

BillThePlatypus commented 4 years ago

I would disagree with renaming the package, although restructuring could be worthwhile.

I think these plugins are for ROSflight, because they simulate the sensors, without all of the overhead of the SIL simulation. To ROS, a simulation with these plugins looks largely the same as a hardware aircraft with a flight controller connected to rosflight_io. An estimator or controller written with rosflight_plugins works the same with hardware.

Additionally, I think that naming it ROSmav_plugins would make it less apparent that it is meant to work with ROSflight and the ROSflight ecosystem. Rather, it would appear to be related to MAVROS, which it isn't. You could choose another name, but I think it is too closely related to ROSflight to not have it in the name.

I think restructuring could be a good idea, however. The three simulation packages in the ROSflight ecosystem (rosflight_sim, roscopter_sim, and rosplane_sim) are all included with their other packages (rosflight, roscopter, and rosplane). This is inconvenient for systems without Gazebo, particularly on-board computers for aircraft. It would be nice to install simulation packages separately.

I think that the issues with version incompatibility with roscopter_sim/rosplane_sim and rosflight_sim/rosflight_plugins are not a huge issue. Any significant changes to rosflight require corresponding changes in roscopter and rosplane, including simulation packages. These changes are infrequent. Most recently, this was done with the ROSflight GNSS changes. If changes are not reflected in the simulator, the value as a simulator is diminished. Probably the better solution is to ensure that changes are coordinated, and merged with master on all relevant repositories at the same time. Perhaps some other improvements in version control are in order, such as having numbered releases.

mmmfarrell commented 4 years ago

The simulation packages being included in roscopter and rosplane shouldn't be an issue because they are only built if gazebo is detected. I know this is the case for roscopter_sim and rosflight_sim, not sure about rosplane_sim.

I see your point about the naming. I definitely think that numbered releases is a good idea. If we do not put this repo as a submodule to roscopter and rosplane, then I think that numbered releases with appropriate documentation (i.e. roscopter version 1.0 has been tested with rosflight_plugins version 2.5, etc) would also work. I'm just afraid that the appropriate documentation will not be maintained.

BillThePlatypus commented 4 years ago

We might be able to do something in github to require specifying min/max versions for the software.

Failing that, it would at least be useful because it would allow communication, e.g.: "rosflight_sim 1.6+ is known to be incompatible with roscopter_sim below 1.4" "This simulation was tested with rosflight 1.4, roscopter 1.3, and ..."

Worst case, you could grab a version of roscopter, and find the most recent rosflight when that was released. Not optimal, but it works.

This is not a new or unique problem. Is there an existing best practice for this?

dpkoch commented 4 years ago

With the naming issue, it may be helpful to identify why rosplane_sim and roscopter_sim exist and are maintained separately from rosflight_sim, instead of ROSplane and ROScopter just using rosflight_sim as their simulator.

Correct me if I'm wrong, but I believe the reason that ROSplane and doesn't just use rosflight_sim is because the vision for ROSplane was to replicate the UAV book as closely as possible, implementing that simulation environment as closely as possible, which precludes dealing with all the SIL stuff. And roscopter is following the pattern of ROSplane. So it seems to me that rosflight_plugins, which supports rosplane_sim and roscopter_sim, has nothing really to do with ROSflight proper, just ROSplane and ROScopter. So I would argue in favor of renaming to make that separation more clear. Either that or looking into consolidating.

The versioning issue is tricky; I'm not sure what the best practice would be. For the ROSflight stuff at least, we need to get this next (pretty significant) release out, then definitely do better about releasing more often with smaller incremental changes.