From bcd5a30ef286ef601f65bd1f0e06b13dc88fd132 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Thu, 22 Oct 2026 10:53:14 +1000 Subject: [PATCH] fix: ROSBag detects the ROS release by default (release="auto") ROSBag defaulted to release="ROS1_NOETIC", while bagtool's --release already defaulted to "auto". The Noetic default could not read ROS 2 (CDR) messages: on a rosbag2 directory ROSBag(path) raised a SerdeError, and a bag whose messages the Noetic definitions do not match read nothing. "auto" tries the known releases on the first message and keeps the first that decodes it, for ROS 1 and for CDR messages; the choice is then available as the `release` attribute. - The default is "auto". Naming a release still works and skips detection. - The "auto-detect: ..." line it printed when it succeeded is now shown only with verbose=True, since it would otherwise be printed for every bag. The "FAILED ... no matching typestore found" message is still always printed. - The docstring now explains what `release` selects and how "auto" works. Adds tests/test_ros_release_auto.py (8 tests, 6 fail on the old default): the default, the same images as the right explicit release for the ROS 1 bag and the ROS 2 bag directory, the detected release being reported, and quiet unless verbose. Co-Authored-By: Claude Sonnet 5.5 --- src/machinevisiontoolbox/Sources.py | 30 +++++++---- tests/test_ros_release_auto.py | 82 +++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 10 deletions(-) create mode 100644 tests/test_ros_release_auto.py diff --git a/src/machinevisiontoolbox/Sources.py b/src/machinevisiontoolbox/Sources.py index 6ad9fd08..6675221c 100644 --- a/src/machinevisiontoolbox/Sources.py +++ b/src/machinevisiontoolbox/Sources.py @@ -2994,7 +2994,7 @@ class ROSBag(ImageSource): :param filename: path to a ``.bag`` file, or an ``http(s)://`` URL :type filename: str or Path - :param release: ROS release name (or unique substring), e.g. ``"noetic"``, defaults to ``"ROS1_NOETIC"``; controls the message definitions used to parse the bag file + :param release: ROS release (distribution) whose message definitions are used to parse the bag file, a name or unique substring such as ``"noetic"`` or ``"humble"``, or ``"auto"`` to detect it, defaults to ``"auto"`` :type release: str :param topicfilter: only yield messages from this topic or list of topics; ``None`` accepts all topics @@ -3095,12 +3095,20 @@ class ROSBag(ImageSource): referenced by their bare name, e.g. ``ROSBag("forest.bag")``. .. note:: - The ``release`` argument controls the ROS message definitions used to - parse the bag file. This is important because message definitions can - change between ROS releases, and using the wrong definitions can lead to - incorrect parsing of the data. The ``release`` string is matched - case-insensitively against the member names of :class:`~rosbags.typesys.Stores`, - so short names such as ``"noetic"`` or ``"humble"`` are accepted in addition to the full enum name (e.g. ``"ROS1_NOETIC"``). + The ``release`` argument selects the ROS release (distribution) whose + message definitions are used to parse the bag file. Message definitions + can change between releases, and using the wrong ones can lead to + incorrect parsing of the data. + + The default ``"auto"`` tries the known releases on the first message of the + bag and keeps the first that decodes it, for both ROS 1 and ROS 2 (CDR) + encoded messages. The choice is then available as the ``release`` + attribute. Pass ``verbose=True`` to see what was tried. + + Alternatively name a release. The string is matched case-insensitively + against the member names of :class:`~rosbags.typesys.Stores`, so short names + such as ``"noetic"`` or ``"humble"`` are accepted in addition to the full + enum name (e.g. ``"ROS1_NOETIC"``). """ @@ -3115,7 +3123,7 @@ class ROSBag(ImageSource): def __init__( self, filename: str, - release: str = "ROS1_NOETIC", + release: str = "auto", topicfilter: str | list[str] | None = None, msgfilter: str | list[str] | None = "Image", dtype: np.dtype | str | dict | None = None, @@ -3530,7 +3538,8 @@ def _autodetect_typestore(self, rawdata: bytes, msgtype: str) -> bool: store.deserialize_ros1(raw, msgtype) self.typestore = store self.release = name - print(f" auto-detect: {name} (ros1 encoding)") + if self.verbose: + print(f" auto-detect: {name} (ros1 encoding)") return True except (*_LENGTH_ERRORS, KeyError, UnicodeDecodeError, struct.error) as e: # wrong store, or the message is the wrong length for it @@ -3562,7 +3571,8 @@ def _autodetect_typestore(self, rawdata: bytes, msgtype: str) -> bool: self.typestore = store self.release = name self._is_ros2 = True # use CDR for all subsequent messages - print(f" auto-detect: {name} (CDR encoding)") + if self.verbose: + print(f" auto-detect: {name} (CDR encoding)") return True except ( *_LENGTH_ERRORS, diff --git a/tests/test_ros_release_auto.py b/tests/test_ros_release_auto.py new file mode 100644 index 00000000..0534b7dd --- /dev/null +++ b/tests/test_ros_release_auto.py @@ -0,0 +1,82 @@ +"""ROSBag detects the ROS release by default (``release="auto"``). + +``bagtool --release`` already defaulted to ``"auto"``; the ``ROSBag`` class defaulted to +``"ROS1_NOETIC"``, which cannot read ROS 2 (CDR) messages. +""" + +from __future__ import annotations + +from pathlib import Path + +import numpy as np +import pytest + +pytest.importorskip("rosbags") + +from machinevisiontoolbox import ROSBag + +DATA = Path(__file__).parent / "data" +ROS1_BAG = DATA / "test_ros1.bag" +ROS2_BAG = DATA / "test_ros2" + +# (bag, a release that is known to be right for it) +BAGS = [(ROS1_BAG, "noetic"), (ROS2_BAG, "humble")] + + +def _need(bag: Path) -> None: + if not bag.exists(): + pytest.skip(f"fixture {bag.name} not generated") + + +def test_default_release_is_auto(): + _need(ROS1_BAG) + assert ROSBag(str(ROS1_BAG)).release == "auto" + + +@pytest.mark.parametrize("bag, release", BAGS) +def test_default_reads_the_same_images_as_the_right_release(bag, release): + _need(bag) + auto = list(ROSBag(str(bag))) + explicit = list(ROSBag(str(bag), release=release)) + + assert len(auto) == len(explicit) > 0 + for a, e in zip(auto, explicit): + assert a.topic == e.topic and a.timestamp == e.timestamp + np.testing.assert_array_equal(a.array, e.array) + + +@pytest.mark.parametrize("bag, release", BAGS) +def test_detected_release_is_reported(bag, release): + _need(bag) + reader = ROSBag(str(bag)) + assert reader.release == "auto" + next(iter(reader)) + assert reader.release != "auto" + assert reader.release.upper().startswith("ROS") + + +@pytest.mark.parametrize("bag, release", BAGS) +def test_detection_is_quiet_unless_verbose(bag, release, capsys): + _need(bag) + list(ROSBag(str(bag))) + assert "auto-detect" not in capsys.readouterr().out + + list(ROSBag(str(bag), verbose=True)) + assert "auto-detect" in capsys.readouterr().out + + +def test_an_explicit_release_still_works_and_is_not_changed(): + _need(ROS1_BAG) + reader = ROSBag(str(ROS1_BAG), release="noetic") + assert len(list(reader)) > 0 + assert reader.release == "noetic" # nothing was detected, nothing to report + + +def test_default_reads_a_real_ros2_mcap_bag(): + """The old default, ROS1_NOETIC, raised a SerdeError on this recording.""" + path = Path(__file__).parent.parent / "bags" / "ros2_bouncing_ball" + if not path.exists(): + pytest.skip("real ROS 2 bag not available") + images = list(ROSBag(str(path))) + # The bag's metadata claims 132 + 133 images, but its files only hold 39 + 40. + assert len(images) == 79