Open mistoll opened 8 years ago
Hi Michael. I'm sorry for the slow reply - I knew it would take a little while to look at what you'd done and assess it.
High level - I would love some help maintaining this, and I don't know how to release packages into ROS (jade, kinetic debs) but clearly this needs to happen. I'm flat out at work and it's preventing me from doing the maintenance that is needed, and I will be supportive to the extent I have time. But I'd love to see this component moved forward and into the mainstream of deb packages that can just be installed.
Are you proposing to become THE maintainer? I think I'm good with that - you have taken an interest and have merged pull requests from a couple of other contributors - all good signs. I guess this means your fork would become THE fork that the wiki page would point to. Are you good with maintaining the wiki page too? I put a lot of work into it and I believe in good documentation. It would need to be updated when the debs get released to tell how to get them and what to do about libsbp.
As far as you developing and pushing the deb package out to the community, I definitely welcome that - I don't see that I'll get time to do that in the near future.
A couple of technical comments:
Overall, the split seems to me to make two very tiny packages and one big one out of the current single package, and they're all closely coupled, and it's more work to maintain/release 3 packages than one, and I don't see compensating benefits. Have you done a deb release? It's a pain in the behind - a lot of slow grinding - and you're tripling the amount of that that has to be done.
I didn't look closely at the other changes but they all seem like goodness.
Let me know what you think.
Regards
Paul
IMHO addressing the libsbp package issue would be much more valuable than splitting the current package into three. It's a real issue.
I already dealt with the libsbp problem. See the libsbp-release package. This isn't in the ros distribution, yet. (Made with the ReleaseThirdParty tutorial)
The reason I've splitted the piksi_msgs package is obsolete. This can be merged into piksi_driver.
IMO the "tutorials" should be kept separate. Anyone using the driver would need a different setup here. While it is a good starting point, changes to the configuration are very likely. Especially the diagnostic_aggregator is a node, which is used for other subsystems, too. So while it is definitiely usefull, the launch file is an example and should not be part of the core package.
I don't understand why you think the release of an additional package is so much work. The release is done per repository, not per package. And it's well automated. So the work is almost the same for a repository containing 10 packages as a repository containing a single package. (Release tutorial )
I'm not pushing to become the maintainer. However the package and the release repository should be managed by the same person. Of course I'd update the wiki page, when releasing. But I don't have too much time for this, too.
Well, I'm impressed that you got the ReleaseThirdParty process to work - I tried and failed. Clearly you are smarter than me :-). I'm glad you are open to merging piksi_msgs back with the driver.
I understand your points about the diagnostic_aggregator and how it shouldn't be part of the package. But how then should the driver publish diagnostics/telemetry? Or do you think it shouldn't?
Re the launch file, there seems to be a practice of including at least one launch file in a package that starts it in a typical configuration - for example the usb_cam package that I worked with today has a launch file that starts the node with typical parameters and also starts a viewer - it makes it easy to get started. I just needed to copy its launch file & change the one parameter I cared about and it just worked. I can see removing the diagnostics .yaml files, but I suggest having at least a basic "start it up with typical parameters" launch file.
I didn't realized a release is per repo, & not per package - you know much more than me about this.
I agree that the package and the release repo should be managed by the same person. I'm happy to turn over maintainership to you, or add you as a maintainer - either way. And since you are willing to release it, just tell me what I have to do to facilitate that. I'm really very very happy that you are interested in this package and I'll do anything that will help it thrive and grow - thank you very much for being willing to contribute.
Ok, I'm going to clean up my whole fork. Maybe I can event merge some improvements from other forks. This will take some time.
I agree, the launch and config files are good examples and I wouldn't remove any of them. However I don't like them in the same package, because then, they would also exist on any installation on a productive system, where there's usually custom launch files. I'll have a look at the other forks around. If they are using all the same launch file, it's good to go into a single package. If they all changed the launch file to their own requirements, we should put it in a different package.
When using multiple packages there should also be a metapackage, so we can install all of them with a single package name.
I'll contact you before releasing, when everything is cleaned up. We can discuss further topics, then.
I've created a fork of this repo and added some features. Are you willing to pull those changes into your repo? (I'd prepare a PR)
And I'd like that to be released into ROS. There wasn't much traffic here, lately. Do you want to keep maintainership? Are you willing to release this package? If you don't want to, I'll do it.