Allow for proper error handling #340
Conversation
* Pass kNumResiduals to the internal AutoDiff function. Ceres added this argument in ceres-solver/ceres-solver@e7a3035 we need to pass it else template substitution fails. * Pass kLocalSize instead of kGlobalSize Upstream commit made me assume kGlobalSize, but that threw at runtime when the tests ran. This seems to work, also put a using statement there to make roslint happy. Co-authored-by: Ivor Wanders <[email protected]>
* Update to C++17 for use with Ubuntu Jammy * Include Rviz and Eigen as system includes, which supresses warnings within the included libraries * use pluginlib and class_list_macros .hpp include instead of deprecated .h From: Lucas Walter <[email protected]>
* Use fuse_macros.h instead of deprecated macros.h * Add missed header * Sort headers
* Add geometry_msgs as a test_depend. * Make geometry_msgs includable. --------- Co-authored-by: Ivor Wanders <[email protected]>
| @@ -105,7 +106,7 @@ class NormalPriorOrientation3DEulerCostFunctor | |||
|
|
|||
| for (size_t i = 0; i < axes_.size(); ++i) | |||
| { | |||
| T angle; | |||
| T angle = T(0); | |||
There was a problem hiding this comment.
This is here to satisfy compiler warnings about angle potentially not being defined after the introduction of ErrorHandler meant that code that actually uses angle could be reached without angle being guaranteed to have been set. Recall that the ErrorHandler call is not always guaranteed to throw an exception
There was a problem hiding this comment.
I'm trying to get my head wrapped around how this would be used in practice. The following is sort of a "stream of consciousness".
You have a specific set of "context" strings. I really like the idea of being able to select different error handlers for different contexts. It would be fantastic if the end user could create their own contexts for their own work. But generic, extensible everything isn't always possible. And the list of the contexts you provide does allow quite a bit of customization.
It would be nice if the "generic" context acted as a fallback. For example, I write a CustomVariable class and throw an error with the VARIABLE context. If I haven't registered a VARIABLE error handler, it would be nice if the GENERIC error handler was used instead.
All of the errors are "thrown" in the code with a fixed context. Right now, all of fuse is using the GENERIC context. I wonder if the context should be set to the most specific context available? So the user is able to handle fuse VARIABLE errors differently if they want to. This is somewhat coupled to previous point about having a fallback handler.
A fallback system reminds me somewhat of a series of try-catch-catch-catch clauses. Wouldn't it be neat if the fuse code could pass an exception object to the error handler, and then the user could write a series of catch blocks to handle different exceptions? The default would be to catch std::exception, which would handle everything. But if the user wanted to create their own exception class, they could catch that exception type explicitly and do something special. And fuse could define a suite of exception classes to help with that. E.g. std::exception --> fuse::exception --> fuse::variable_exception --> fuse::orientation_variable_exception kind of thing. And then the handler could catch at whatever class hierarchy level they want. The locomotor (or related package) does something like this for the recovery behaviors....somewhere that I cannot find. But it uses std::exception_ptr to move exceptions around.
As I said, this is not a review exactly. Just some random thoughts thrown together for you to react to.
Practically speaking, my thoughts were that this would be most useful with a lot of bind expressions for callbacks, as I figure that's most likely to give you an actually useful error handler in the event you want to do something other than just throw an exception.
Minor nitpick: The context list is specifically an enum so that if anyone wants to add a new context in, all they have to do is drop it in at the end of that list. The existing list is really only there because I looked at the list of existing namespaces and said "Hey, these roughly correspond to our existing major building blocks, let's add these as handler contexts." I think this is about as much extensibility as we're going to get on that front.
This I can do, and makes sense to do. I will add it
This I'm not as sure of. I know I did, in fact, name all of the contexts after namespaces, but I don't necessarily want to establish that the namespace context should always necessarily be used in a namespace. My view is that more specific contexts should be used if and when a dev wants/needs to do so.
I've read through this a couple of times now, and you're right that this would be kind of neat, but I don't necessarily see what the benefit of this would be over the current approach. It mostly seems like it would lead to a ton of branching exception classes for not a ton of benefit. |
Right. So this may be a difference between making it work for Locus and making it work for the community. You are a user of the fuse library at David's Robot Emporium, and you want to add a new sensor model. When you do that, you would like to add a new error handler group. However, you cannot add a new item to the enum class without forking the fuse repo and maintaining your own customized copy. And forcing people to fork the repo is something I've been trying really hard to avoid. This is why my mind jumped to exceptions as a possible alternative. The end user could create their own derived exception class without modifying the open-source fuse repo: And then the user could write their own error handler that catches that exception type and does something special. Or it falls through to the SensorException handler, or the generic FuseException handler. And that hierarchy of error handler types and fallbacks could be made arbitrarily complex by the end user without requiring any changes to the open-source code. That's what I was thinking anyway. |
As Fuse currently stands, there's nothing in Fuse that handles errors in a sane way. Any possible errors are handled solely by throwing exceptions and hoping that they're caught. This PR aims to fix that by adding an error handler class, where a variety of callbacks can be registered and called as needed to handle errors as they come up. It also provides the ability to define different error handlers for various classes of errors based on a given context.
This PR looks nasty, but most if it
A couple of notes:
This is inherently synchronous, as Fuse currently assumes that any errors are handled synchronously, and I think it would be a bit much to change that. Anyone who wants to is free to define an AsyncErrorHandler and deal with asynchronous errors themselves.
As a result of it being synchronous, any callbacks provided need to make sure that they're thread safe if necessary.
By default, the error handler preserves the current default behaviour of throwing exceptions whenever an error is found. Anything beyond that needs to be explicitly set.