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

Add exception-safe version of serial_driver #77

Open
RFRIEDM-Trimble opened this issue Feb 7, 2022 · 2 comments
Open

Add exception-safe version of serial_driver #77

RFRIEDM-Trimble opened this issue Feb 7, 2022 · 2 comments

Comments

@RFRIEDM-Trimble
Copy link
Contributor

RFRIEDM-Trimble commented Feb 7, 2022

As found during my implementation of serial_break in serial_port.hpp, I found that many of the asio functions used can throw, unless they are supplied an optional error code by reference.

The problem:

  1. The current documentation does not indicate which functions are exception safe. As a consumer of the SerialPort class, I have to look through the source code to see what throws and then add explicit catch statements.
  2. The coding standards I use recommend catching exceptions early by instead bringing them up to the higher layers (ie, ROS logger or ROS diagnostics) through the use of error codes. For a real-time system, not throwing exceptions could make it more deterministic timing-wise.

The proposal:

I am happy to help with design and implementation for this and contribute the changes open source, but could use some guidance. Key things to me:

  1. Clarify through the code comments on which functions can throw in the current serial_port.hpp file.
  2. Create a design (separate driver, separate function calls) to support an exception-safe version of the driver or modify the current classes to support this.
  3. Design appropriate unit tests to ensure all calls to asio functions that can throw are appropriately protected.
  4. At this point, it may be worth focusing on run-time exceptions rather than exceptions during the bring-up of the port, such as trying to open a port that doesn't exist versus reading from a port that was just disconnected during runtime.
@JWhitleyWork
Copy link
Contributor

@RFRIEDM-Trimble I think the proposal is reasonable and there is nothing wrong with making the current code exception-safe during runtime. The constructors and connect/disconnect functions could have different signatures to indicate whether the user wants to run the exception-safe versions and grab an error code or use the exception-capable versions and use try/catch (or not). Happy to review any PRs to this affect though they should be targeted at Galactic at a minimum (preferrably Rolling) since this will affect the external API of the library.

@RFRIEDM-Trimble
Copy link
Contributor Author

Thanks!

Yes, we use Galactic. Based on what you said, it seems you want me to modify the existing class to add overrides?

I could also not modify existing signatures, but do augment existing documentation when functions have the potential to throw.

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

No branches or pull requests

2 participants