[RSDK-11250, RSDK-11251, RSDK-11784, RSDK-11807] - Cpp improvements#27
[RSDK-11250, RSDK-11251, RSDK-11784, RSDK-11807] - Cpp improvements#27seanavery merged 14 commits intoviam-modules:mainfrom
Conversation
| // Every Viam C++ SDK program must have one and only one Instance object | ||
| // which is created before any other C++ SDK objects and stays alive until | ||
| // all Viam C++ SDK objects are destroyed. | ||
| Instance inst; |
There was a problem hiding this comment.
Need to move Instance before any logger use
|
|
||
| // Getters | ||
| std::string get_name() const; | ||
| bool is_debug() const; |
There was a problem hiding this comment.
Removing debug attribute
| if (image.bytes.empty()) { | ||
| std::cerr << "ERROR: no bytes retrieved" << std::endl; | ||
| VIAM_SDK_LOG(error) << "no bytes retrieved from get_csi_image"; | ||
| throw Exception("no bytes retrieved from get_csi_image"); |
There was a problem hiding this comment.
would std::runtime_error be more appropriate of an error to throw?
I'm gonna @ Lia @lia-viam
There was a problem hiding this comment.
honestly not super important given the way we handle exceptions. note that viam::sdk::Exception is a public derived class of runtime_error, which matters in situations where you'd be interested in granular filtering on the exception type. You can skip the java stuff but see here: https://www.geeksforgeeks.org/cpp/catching-base-and-derived-classes-as-exceptions-in-cpp-and-java/
the distinction between Exception and runtime_error might matter if we were writing catch (const viam::sdk::Exception&) blocks and doing something meaningful with them, which we are not currently
the thing I would suggest though is rather than logging a message then putting it in an exception, in main.cpp you can wrap the call to serve in a try/catch block, like
try { my_mod.serve(); }
catch (const std::exception& e) {
VIAM_SDK_LOG(err) << e.what();
return EXIT_FAILURE;
}There was a problem hiding this comment.
@lia-viam Thanks for the explanation. I can wrap serve in a try catch, however, I thought that a throw here would get swallowed somewhere in the SDK/GRPC layer... Are you saying std::runtime_error will bring down the module server? This path gets hit behind GetImage/GetImages GRPC calls.
There was a problem hiding this comment.
@lia-viam Should I be using GRPCException here instead? My intention is just to error out on the GRPC call.
There was a problem hiding this comment.
It looks like the orbbec module is hitting std::runtime_error (which would be similar to the viam::sdk::Exception use here) behind GRPC calls. See get_image error handling.
There was a problem hiding this comment.
hey sean sorry I missed the ping. so my concern here was more just the duplication of throwing an error and having to log the same message. if you're implementing a method on a component then yes that gets handled by the grpc layer, although I think it should result in SDK or RDK (I forget which) logging the text of the exception you threw, so it's maybe not necessary to duplicate the logging.
for exception throws not in a component method implementation (or transitively called by one) then yes a try/catch in main would take care of that. this can indeed be more helpful than letting RDK take care of it, because often if you terminate with an uncaught exception you just get a message like "std::terminate called, uncaught exception of type runtime_error" without printing the message.
The main takeaway is it doesn't matter a ton whether you're throwing Exception or runtime_error, but no I wouldn't recommend throwing GRPCException
There was a problem hiding this comment.
if you're implementing a method on a component then yes that gets handled by the grpc layer
Makes sense, thanks.
for exception throws not in a component method implementation (or transitively called by one) then yes a try/catch in main
Ok, added try/catch block around the main function to handle this case better.
Also, removed dup logging, now allowing GRPC swallow to spit out the msg.
SebastianMunozP
left a comment
There was a problem hiding this comment.
These changes look good to me.
|
Description
Testing