➕ IPC functionality #5
Conversation
include/spdlog/LICENSE
Outdated
| @@ -0,0 +1,25 @@ | |||
| The MIT License (MIT) | |||
There was a problem hiding this comment.
Please scroll down (to init.d/ocvsmd file) all these include/spdlog/* files - this is the logging 3rd-party headers. Currently, I dropped them right into repo, but we might reconsider to make it as a submodule. In the next PR there will be one more dependency - headers only TOML parsing library - daemon configuration will be loaded/saved to /etc/ocvsmd/ocvsmd.toml file).
There was a problem hiding this comment.
Surely these includes need to be nested into a directory whose name makes it clear that these are third-party components. 3rdparty sounds like a much better name.
There was a problem hiding this comment.
@pavel-kirienko So, under include/3rdparty/spdlog/*, right?
@thirtytwobits do you agree?
There was a problem hiding this comment.
Yeah. We should have these elsewhere. Why not a submodule or Cmake external project?
There was a problem hiding this comment.
This is header-only, right? We don't want any dynamic dependencies with this solution.
There was a problem hiding this comment.
Yes, headerS-only (plural). No dynamic dependencies - neither this SPDLOG nor next pr TOML parsing library.
There was a problem hiding this comment.
SPDLOG now as a submodule.
|
|
||
| } // namespace detail | ||
|
|
||
| /// Abstract interface of a result sender. |
There was a problem hiding this comment.
These sender and receivers are inspired by the P2300 c++ proposal. Not even close of course, but the main idea is that client of our SDK could compose senders (using .then( callback ) or | operator) to build necessary async pipeline, and then sync_wait to wait for result (see src/cli/main.cpp:94).
There was a problem hiding this comment.
I was not aware of that proposal. I'm wondering if libcyphal executors and callbacks should be conformed to harmonize with these concepts?
There was a problem hiding this comment.
most probably. but it's still kinda experimental. will see how it goes with next file server functionality of the SDK - this is where I will experiment (in my cli tool) with the composing, like several async sdk calls:
- add some root path(s) to remote file server
- launch software update
- somehow wait for completion (not sure how though...)
- remove the paths from the file server
| @@ -0,0 +1,78 @@ | |||
| # This is copy/paste of the request part of the `uavcan/node/435.ExecuteCommand.1.3.dsdl` service definition. | |||
There was a problem hiding this comment.
These are copy/paste from corresponding public DSDL types - unfortunately it's impossible (currently) to reference request or response part of a service inside another dsdl type. @pavel-kirienko said it could be relatively easy to add such support (to pydsdl / nunavut ) in the future. If it happens then we will drop these copies.
There was a problem hiding this comment.
I think this change won't even affect Nunavut at all; it can be confined to PyDSDL only, as the final AST will look the same.
There was a problem hiding this comment.
Love that we're learning and growing Cyphal here!
| class FileServer | ||
| { | ||
| public: | ||
| // `errno`-like error code (or anything else we might add in the future). | ||
| using Failure = int; // or `cetl::variant<int, ...>` |
There was a problem hiding this comment.
Using numbers for error codes is an anti-pattern
There was a problem hiding this comment.
I would like to agree, but honestly... I don't like these "monstrous" failure variants at libcyphal either.
There was a problem hiding this comment.
I agree with both of you. The failure variants are a bit much to handle. Raw integers are delicate. Here's a pattern we use at Prime Air that is both easy but also somewhat structured:
First we decide that at least 4-bytes is needed
static_assert(sizeof(unsigned) >= 4, "32-bits or more is required to compile this software. Sorry AT-TINY.");
then we break up the bytes into some error classes
constexpr std::size_t const ArgumentClassMask = 0xFF;
constexpr std::size_t const ArgumentErrorShift = 8U;
constexpr std::size_t const MemoryErrorShift = 16U;
constexpr std::size_t const FSMErrorShift = 24U;
next we define a scoped and typed enumeration
///
/// * 0x0000000 : success / no-errors
/// * 0x0000001 - 0x000000FF : general error codes
/// * 0x0000100 - 0x0000FF00 : argument error codes
/// * 0x0010000 - 0x00FF0000 : memory error codes
/// * 0x1000000 - 0xFF000000 : finite state-machine error codes.
///
/// While these memory regions don't overlap as defined, each Result value is a discrete enumeration value and
/// cannot contain multiple error codes (i.e. this is _not_ a bitfield).
enum class Result : unsigned {
// --[SUCCESS]-------------------------------------------------------------------------------------------
kSuccess = 0U, ///< No errors.
// --[GENERAL ERRORS]----------------------------------------------------------------------------------
kFailureGeneric = 1U, ///< A generic failure occurred (no specific reason
///< is known). This might be returned from some
///< external code or because the author of a
///< method was just too lazy to think about the
///< value any harder than this.
kFailureBusy = 2U, ///< The operation was busy and on-going. It may be
...
// --[ARGUMENT (PARAMETER) ERRORS]--------------------------------------------------------------------
kArgumentErrorInvalidValue = 1U << ArgumentErrorShift, ///< The value provided was invalid
kArgumentErrorInvalidSize = 2U << ArgumentErrorShift, ///< The size provided was invalid
...
// --[MEMORY (RESOURCE) ERRORS]-------------------------------------------------------------------------
kMemoryErrorInvalidAddress = 1U << MemoryErrorShift, ///< The address provided was invalid or null.
kMemoryErrorInvalidSize = 2U << MemoryErrorShift, ///< The size of the memory was invalid, likely zero
...
etc.
Finally some helpers:
constexpr bool IsSuccess(Result const& result) {
return to_underlying(result) == 0x00;
}
constexpr bool IsError(Result const& result) {
return to_underlying(result) != 0x00;
}
constexpr bool IsArgumentError(Result const& result) {
return (to_underlying(result) & (ArgumentClassMask << ArgumentErrorShift)) != 0;
}
constexpr bool IsMemoryError(Result const& result) {
return (to_underlying(result) & (ArgumentClassMask << MemoryErrorShift)) != 0;
}
constexpr bool IsFsmError(Result const& result) {
return (to_underlying(result) & (ArgumentClassMask << FSMErrorShift)) != 0;
}
And voila. Super easy to return something like Result::kSuccess and test IsSuccess(result) but you can also branch on error class. For example:
if (IsError(result)) {
if (isArgumentError(result)) {
handleArgumentErrors(result);
} else if (isMemoryError(result)) {
handleMemoryErrors(result);
} else if (isFsmError(result)) {
handleFsmErrors(result);
} else {
handleUnknownErrors(result);
}
}
There was a problem hiding this comment.
there is also c++ <system_error> facility with its error categories, codes and conditions.
There was a problem hiding this comment.
I don't get the point of Scott's idea. We dislike algebraic types because they are cumbersome to handle in C++ due to language's limitations; but then the enum approach also comes with boilerplate in the form of the helper functions. One good thing about variants is that they allow one to arbitrarily customize the auxiliary information provided with each error type, something that is not attainable with numeric errors.
Perhaps we could provide helper functions for our error variants instead, or polyfill std::expected as we planned to do for a while?
There was a problem hiding this comment.
allow one to arbitrarily customize the auxiliary information provided
Yes, that is a good thing. What is also good is that if one extends set of possible errors with a new kind/source then most probably related error handling code start fail to compile. One might argue whether it's good or bad... for instance such addition will be a breaking change. IMO it's more good than bad - b/c you have compile time prove that ALL possible cases are handled.
But, at the same time, if we look f.e. at the libcyphal failures, in reality we have only ONE such case (with PlatformError and its attached errno-like error code) - ALL other numerous errors are just empty structs. and in the whole libcyphal we don't have even a single place where we really analyze or make some business logic decision based on what specific error has happened - instead we kinda "wash our hands" and pass it as is to the user provided transient error handler, and let client to deal with it.
There was a problem hiding this comment.
That is expected -- the user's callback is supposed to have the right context to handle errors sensibly.
There was a problem hiding this comment.
That is true. But for me, who already wrote several demos and now ocvsmd, honestly it was not fun to write such callbacks... I believe that users might be tempted to go with "swallow all" (with passing [] (auto&) { ... } to std::visit) - this will kill the good thing I described above (about "prove that ALL possible cases are handled").
There was a problem hiding this comment.
I think the benefits of a strongly-typed design outweigh the inconveniences caused by the limited expressivity of the language. In the future we should be able to improve this by switching to better algebraic containers, like std::expected.
docs/file_server.hpp
Outdated
| /// so the list will be automatically restored on the next daemon start. | ||
| /// Returns `cetl::nullopt` on success. | ||
| /// | ||
| virtual cetl::optinal<Failure> push_root(const cetl::string_view path, const bool back); |
There was a problem hiding this comment.
| virtual cetl::optinal<Failure> push_root(const cetl::string_view path, const bool back); | |
| virtual cetl::optional<Failure> push_root(const cetl::string_view path, const bool back = true); |
There was a problem hiding this comment.
typo will be fixed but regarding default argument for a virtual function parameter there are following:
include/spdlog/LICENSE
Outdated
| @@ -0,0 +1,25 @@ | |||
| The MIT License (MIT) | |||
There was a problem hiding this comment.
Surely these includes need to be nested into a directory whose name makes it clear that these are third-party components. 3rdparty sounds like a much better name.
include/spdlog/LICENSE
Outdated
| @@ -0,0 +1,25 @@ | |||
| The MIT License (MIT) | |||
There was a problem hiding this comment.
Yeah. We should have these elsewhere. Why not a submodule or Cmake external project?
|
|
||
| } // namespace detail | ||
|
|
||
| /// Abstract interface of a result sender. |
There was a problem hiding this comment.
I was not aware of that proposal. I'm wondering if libcyphal executors and callbacks should be conformed to harmonize with these concepts?
include/spdlog/LICENSE
Outdated
| @@ -0,0 +1,25 @@ | |||
| The MIT License (MIT) | |||
There was a problem hiding this comment.
This is header-only, right? We don't want any dynamic dependencies with this solution.
| @@ -14,7 +14,7 @@ enable_testing() | |||
|
|
|||
| set(NO_STATIC_ANALYSIS OFF CACHE BOOL "disable static analysis") | |||
There was a problem hiding this comment.
Protip: use cmake presets to create flavors that turn this on and off.
There was a problem hiding this comment.
Yeah, but I don't want "explode" number of different flavors. that is why f.e. cpp standard is also not part of flavor permutations.
| @@ -0,0 +1,78 @@ | |||
| # This is copy/paste of the request part of the `uavcan/node/435.ExecuteCommand.1.3.dsdl` service definition. | |||
There was a problem hiding this comment.
Love that we're learning and growing Cyphal here!
| class FileServer | ||
| { | ||
| public: | ||
| // `errno`-like error code (or anything else we might add in the future). | ||
| using Failure = int; // or `cetl::variant<int, ...>` |
There was a problem hiding this comment.
I agree with both of you. The failure variants are a bit much to handle. Raw integers are delicate. Here's a pattern we use at Prime Air that is both easy but also somewhat structured:
First we decide that at least 4-bytes is needed
static_assert(sizeof(unsigned) >= 4, "32-bits or more is required to compile this software. Sorry AT-TINY.");
then we break up the bytes into some error classes
constexpr std::size_t const ArgumentClassMask = 0xFF;
constexpr std::size_t const ArgumentErrorShift = 8U;
constexpr std::size_t const MemoryErrorShift = 16U;
constexpr std::size_t const FSMErrorShift = 24U;
next we define a scoped and typed enumeration
///
/// * 0x0000000 : success / no-errors
/// * 0x0000001 - 0x000000FF : general error codes
/// * 0x0000100 - 0x0000FF00 : argument error codes
/// * 0x0010000 - 0x00FF0000 : memory error codes
/// * 0x1000000 - 0xFF000000 : finite state-machine error codes.
///
/// While these memory regions don't overlap as defined, each Result value is a discrete enumeration value and
/// cannot contain multiple error codes (i.e. this is _not_ a bitfield).
enum class Result : unsigned {
// --[SUCCESS]-------------------------------------------------------------------------------------------
kSuccess = 0U, ///< No errors.
// --[GENERAL ERRORS]----------------------------------------------------------------------------------
kFailureGeneric = 1U, ///< A generic failure occurred (no specific reason
///< is known). This might be returned from some
///< external code or because the author of a
///< method was just too lazy to think about the
///< value any harder than this.
kFailureBusy = 2U, ///< The operation was busy and on-going. It may be
...
// --[ARGUMENT (PARAMETER) ERRORS]--------------------------------------------------------------------
kArgumentErrorInvalidValue = 1U << ArgumentErrorShift, ///< The value provided was invalid
kArgumentErrorInvalidSize = 2U << ArgumentErrorShift, ///< The size provided was invalid
...
// --[MEMORY (RESOURCE) ERRORS]-------------------------------------------------------------------------
kMemoryErrorInvalidAddress = 1U << MemoryErrorShift, ///< The address provided was invalid or null.
kMemoryErrorInvalidSize = 2U << MemoryErrorShift, ///< The size of the memory was invalid, likely zero
...
etc.
Finally some helpers:
constexpr bool IsSuccess(Result const& result) {
return to_underlying(result) == 0x00;
}
constexpr bool IsError(Result const& result) {
return to_underlying(result) != 0x00;
}
constexpr bool IsArgumentError(Result const& result) {
return (to_underlying(result) & (ArgumentClassMask << ArgumentErrorShift)) != 0;
}
constexpr bool IsMemoryError(Result const& result) {
return (to_underlying(result) & (ArgumentClassMask << MemoryErrorShift)) != 0;
}
constexpr bool IsFsmError(Result const& result) {
return (to_underlying(result) & (ArgumentClassMask << FSMErrorShift)) != 0;
}
And voila. Super easy to return something like Result::kSuccess and test IsSuccess(result) but you can also branch on error class. For example:
if (IsError(result)) {
if (isArgumentError(result)) {
handleArgumentErrors(result);
} else if (isMemoryError(result)) {
handleMemoryErrors(result);
} else if (isFsmError(result)) {
handleFsmErrors(result);
} else {
handleUnknownErrors(result);
}
}
Note! Everything marked with ➕ symbol will be gradually reverted/uncommented in the next PRs. It's done like this to break all the ready functionality into smaller PRs. This PR is mostly about IPC, but you still can have a sneak peak at what is coming.
org.opencyphal.ocvsmd.ipcname)/var/run/ocvsmd.sock)