locusrobotics / fuse

The fuse stack provides a general architecture for performing sensor fusion live on a robot. Some possible applications include state estimation, localization, mapping, and calibration.
Other
729 stars 122 forks source link

Allow for proper error handling #340

Open DavidLocus opened 1 year ago

DavidLocus commented 1 year ago

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.

DavidLocus commented 1 year ago

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".

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.

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.

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.

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.

This I can do, and makes sense to do. I will add it

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.

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.

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.

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.

svwilliams commented 1 year ago

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.

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:

DavidsSensorException : public fuse::SensorException

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.