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

Broken regex check for SequentialWriter after MCAP merge #1336

Closed
rshanor opened this issue May 17, 2023 · 7 comments
Closed

Broken regex check for SequentialWriter after MCAP merge #1336

rshanor opened this issue May 17, 2023 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@rshanor
Copy link
Contributor

rshanor commented May 17, 2023

Description

I am using rosbag2_py.SequentialWriter class to record service requests and responses to a bag file. Code that previously worked is now throwing a ValueError: Invalid topic_type: pylon_ros2_camera_interfaces/srv/SetGamma_Request. It looks like #1163 added some regex checking to the message type which is not valid for service request and response messages: https://github.com/ros2/rosbag2/blob/rolling/rosbag2_cpp/src/rosbag2_cpp/message_definitions/local_message_definition_source.cpp#L33

>       self.writer.create_topic(topic_info)
E       ValueError: Invalid topic_type: pylon_ros2_camera_interfaces/srv/SetGamma_Request

System (please complete the following information)

  • OS: Ubuntu Jammy
  • ROS 2 Distro: Rolling
  • Version: latest
@rshanor rshanor added the bug Something isn't working label May 17, 2023
@MichaelOrlov
Copy link
Contributor

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented May 18, 2023

@rshanor Could you please clarify what specifically in the service request/responses causing this failure?

@rshanor
Copy link
Contributor Author

rshanor commented May 18, 2023

@MichaelOrlov here is a minimal way to repro.

import rosbag2_py
from std_srvs.srv import SetBool
import tempfile
import uuid


writer = rosbag2_py.SequentialWriter()
storage_options = rosbag2_py._storage.StorageOptions(
    uri=f"/tmp/{uuid.uuid4()}", storage_id="sqlite3"
)
converter_options = rosbag2_py._storage.ConverterOptions("", "")
writer.open(storage_options, converter_options)

msg = SetBool.Request()
msg_type = type(msg)
msg_module = msg_type.__module__
msg_name = msg_type.__name__
msg_mod_split = msg_module.split(".")
topic_type = f"{msg_mod_split[0]}/{msg_mod_split[1]}/{msg_name}"
full_name = "/example"
topic_info = rosbag2_py._storage.TopicMetadata(
    name=full_name,
    type=topic_type,
    serialization_format="cdr",
)
writer.create_topic(topic_info)

@MichaelOrlov
Copy link
Contributor

@emersonknapp @james-rms If you have time, your help with this issue and relevant #1286 will be very appreciated.

@emersonknapp emersonknapp self-assigned this May 19, 2023
@emersonknapp
Copy link
Collaborator

Assigning to myself and looking into it now. This one should be an easy fix, the message definition scraper had a hardcoded /msg/ infix for types. For services, the .msg for _Request and _Repsonse always exist in the install tree, so the data exists.

For #1286 there is the same problem of "can't find the definition" but it's not a complete duplicate, that one deals with Actions which do not have all subtypes as plain files in the install tree.

@emersonknapp
Copy link
Collaborator

The crash scenario was fixed in #1350 (iron backport #1352) - but I'm going to leave this open until #1341 is finished, as that will extend the full feature to services, which is nice to have.

@MichaelOrlov
Copy link
Contributor

  • Closing this issue as fixed
  1. The crash was fixed in the Don't crash when type definition cannot be found #1350.
  2. The follow-up PR Allow for definitions from srvs, as long as the .msg file can be found #1341 was closed as it wouldn't be done. At least for while.
  3. Now we have the special feature and CLI parameters for recording services and no need to workaround it and writing own services recorder for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants