Skip to content
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

Compatibility with both Melodic and Noetic, properly done and tested with Mark III and Mark IV devices #124

Merged
merged 4 commits into from
Apr 27, 2023

Conversation

peci1
Copy link
Contributor

@peci1 peci1 commented Apr 19, 2023

This PR makes the driver compatible with both Melodic and Noetic. I've tested it with a Mark III device using MTData (MTi-G-28A53G35) and a Mark IV device using MTData2 (MTi-30-2A8G4) on both Melodic and Noetic. I've tested both mtnode and mtdevice (configuration, inspection, echo).

TL;DR: I'm willing to become a maintainer of the ROS 1 version of this package (if it's true it is no longer maintained) and do a binary release into Noetic. This PR is well tested and follows the best practices, while doing minimally invasive changes to the project code (eh, CMakeLists.txt got a cleanup). It fully retains backwards compatibility for existing code (except moving mtdef.py out of the nodes/ directory). It doesn't add or change any functionality or behavior.

I know there are already several PRs adding Noetic support. Yet in my view, none of them is perfect (except this one :-D )

  • Python3 support #113 is Noetic-only. It also has a bug in reading MTData legacy messages. And it retains the weird file structure where Python scripts are also modules, which does not work nicely with the rest of ROS. It also does wrong-by-default changes to the serial port settings, assuming HW reset lines are always connected and used.
  • Update for use with Noetic and Python3 #119 is also Noetic-only. It also has the MTData reading bug. It turns the package to the more suitable structure, but it loses the connection between mtdevice.py/mtdef.py before moving and after moving. The PR does some more things that are actually not needed an only clutter the list of changes (like converting mtdef structures to derive from Enum class).
  • add support of noetic #122 is also Noetic-only, but it does not properly update package.xml with the python3-serial dependency. It also has the MTData reading bug. And it adds a change to the reported orientation, which should IMO be discussed separately. This PR also does undiscussed changes to the serial reading timeout.

This PR changes the file structure to properly separate library functionality (moved to a Python package) and script/CLI/node functionality (retained in nodes/ directory). To help keeping the Git history of mtdef.py and mtdevice.py, I've split this PR into multiple commits, which are easier reviewed separately:

  1. Do all the py2 -> py23 changes except moving stuff to a package.
  2. Move (with git mv) mtdef.py and mtdevice.py to a Python package (so that Git and Github figure out the files were moved and offer handy links when viewing history and blaming).
  3. Split script functionality from the moved mtdevice.py back to the nodes/mtdevice.py script. Here, the edit history of nodes/mtdevice.py is lost, but I think that the library functionality is more important to retain edit history than just the CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants