score/mw/diag: Add C++ API for DID & RoutineControl - #4
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
There was a problem hiding this comment.
issue: In general, use more DSL for all types like std::string. For Example:
std::string id;
std::string name;
std::optional<std::string> translation_id;
std::optional<std::vector<std::string>> groups;
can be written as
struct StringLike { constexpr static std::string value };
using Id = StringLike;
using Name = StringLike;
using Group = StringLike;
using TransactionId = StringLike;
struct OptionalTransactionId { constexpr static std::optional<TransactionId> value };
struct OptionalGroups { constexpr static std::optional<std::vector<Group>> value };
Id id;
Name name;
OptionalTransactionId transaction_id;
OptionalGroups groups
There was a problem hiding this comment.
Aliases added... however the proposed struct StringLike { constexpr static std::string value } pattern seems not viable here, requires C++20
Toolchain setup required to compile this PRThe C++ API introduced in this PR depends on I've opened #5 (build: adopt baselibs-style toolchain settings) which aligns What was blocking the build
After applying #5bazel build --config=bl-x86_64-linux //score/...
# INFO: Found 19 targets...
# INFO: Build completed successfully, 77 total actionsAlso noted: two test files without BUILD entries
|
|
High-priority validation update after reproducing locally:
Recommended unblock order:
|
96ac096 to
aa23688
Compare
There was a problem hiding this comment.
thought: This can be better done using standalone types for Startable & Stoppable routine:
class StartableRoutine {
[[no_discard]] virtual auto Start() -> Result<StoppableRoutine>;
};
class StoppableRoutine {
[[no_discard]] virtual auto Stop() -> Result<StartableRoutine>;
[[no_discard]] virtual auto CompletionPercentage() const noexcept -> std::uint8_t;
[[no_discard]] virtual auto GetResult() const noexcept -> Result<ByteVector>;
};
std::unique_ptr<StartableRoutine> CreateRoutine() {
std::make_unique<StartableRoutine>();
}
User application flow:
auto routine = CreateRoutine();
auto routine_running = routine.Start();
auto progress = routine_running.CompletionPercentage()
auto data = routine_running.GetResult()
auto can_restrat_routine = routine_running.Stop();
- This eliminates the accidental stopping of Routine which hasn't started.
- This also eliminates un-necessary std::optional from the API surface and thus avoiding
optional.has_value()checks in the user application code. - Types are stateless and thus the objects created out of them.
- Can be easily adapted to async as it is stateless.
Note: Naming can be better. I may have simplified/generalized/deviated the Routine than how it's used in the domain.
MonikaKumari26
changed the title
9e33ecf to
d6ab1d3
Compare
stmuench
left a comment
stmuench
left a comment
There was a problem hiding this comment.
Oh wait, I missed one thing: we should already prepare these new API for the upcoming design to support CancellationHandlers. But please wait with incorporating my comments until we finished the design [dicussions] for that.
| virtual ~ReadDataByIdentifier() noexcept = default; | ||
| }; | ||
|
|
||
| } // namespace score::mw::diag::uds |
There was a problem hiding this comment.
@MonikaKumari26 as discussed, please roll out the following pattern to RDBI, WDBI, GDI, RoutineControl as well as GenericService (the mw::diag::uds::MetaData type should be but into meta_data.h and can remain an empty struct for the time being):
< ... add include for score::cpp::stop_token ... >
namespace score::mw::diag::uds
{
/// UDS ReadDataByIdentifier service (See ISO 14229-1:2020, Service 0x22).
class ReadDataByIdentifier
{
public:
/// Read raw bytes for the data identifier.
/// < ... ADD param docstrings here ... >
/// @return < ... ADD result docstring here ... >
[[nodiscard]] virtual std::future<ByteVector> Read(const MetaData& meta_data,
score::cpp::stop_token stop_token) = 0;
virtual ~ReadDataByIdentifier() noexcept = default;
};
/// Simplified and synchronous version of the above `ReadDataByIdentifier` (must be non-blocking!)
class SimpleReadDataByIdentifier : public ReadDataByIdentifier
{
public:
/// Read raw bytes for the data identifier in a fast and non-blocking manner.
/// @return Result<ByteVector> on success, NegativeResponseCode on failure.
[[nodiscard]] virtual Result<ByteVector> Read(const MetaData& meta_data) = 0;
virtual ~SimpleReadDataByIdentifier() noexcept = default;
private:
std::future<ByteVector> Read(const MetaData& meta_data, score::cpp::stop_token) final
{
std::promise<ByteVector> promise{};
if (auto result = Read(meta_data); result.has_value())
{
promise.set_value(std::move(result).value());
}
else
{
promise.set_error(result.error());
}
return promise.get_future();
}
};
} // namespace score::mw::diag::udsThere was a problem hiding this comment.
All requested UDS service interfaces have been refactored to match the dual-interface pattern supporting MetaData and stop_token and respective mocks & Bazel build targets have been updated as well.
e198a1a to
38b12dd
Compare
| const MetaData& /*meta_data*/, | ||
| score::cpp::stop_token /*stop_token*/) final | ||
| { | ||
| return HandleMessage(input); |
There was a problem hiding this comment.
it seems you missed the part with the std::promise and std::future that I added
There was a problem hiding this comment.
Fixed !! Added the std::promise / std::future async bridge pattern to all simple adapters.
There was a problem hiding this comment.
I do not see them. Did you upload the respective commit in which you adjusted them?
8859412 to
c1bf452
Compare
9bd3811 to
38b12dd
Compare
| /// @param meta_data Context provided by the diagnostic runtime for this request. | ||
| /// @param stop_token Token that becomes stopped if the runtime cancels the request. | ||
| /// @return std::future<std::Result<score::cpp::blank>> on success, NegativeResponseCode on failure. | ||
| [[nodiscard]] virtual std::future<Result<score::cpp::blank>> Write(ByteView input, |
There was a problem hiding this comment.
Afaik, using score::cpp::blank is discouraged. Could you please double-check that? Since I would expect
| [[nodiscard]] virtual std::future<Result<score::cpp::blank>> Write(ByteView input, | |
| [[nodiscard]] virtual std::future<Result<void>> Write(ByteView input, |
instead here ...
There was a problem hiding this comment.
I intentionally used score::cpp::blank, because SCORE already provides this type as the equivalent of a successful result without return value. My understanding was that Resultscore::cpp::blank makes the success semantics explicit while still allowing error propagation through Result.
That said, if the current guideline is to prefer Result over Resultscore::cpp::blank, I'm happy to align with that recommendation. Please let me know..
There was a problem hiding this comment.
Replaced deprecated score::cpp::blank with Result
…k> and update RoutineControl
579b452 to
4fe766f
Compare
|
|
||
| /// Result type: either a value T or a UDS NegativeResponseCode. | ||
| template <typename T> | ||
| using Result = score::Result<T>; |
There was a problem hiding this comment.
where is the part <...> or a UDS NegativeResponseCode specified? I do not see any reference to our uds::NegativeResponseCode here!
There was a problem hiding this comment.
score::cpp::expected<T, NegativeResponseCode> was lacking void specialization in score_baselib. score::Result<T> was only to valid to use Result<<void>>. Now with this it is required to convert NegativeResponseCode to score::result::Error using score::MakeUnexpected(..)
Other option is keep score::cpp::expected<T, NegativeResponseCode> and add void specialization in our diag_result.h until base-lib has this.
There was a problem hiding this comment.
I see. But do we have any unit tests which verify that error construction actually works in conjunction with uds::NegativeResponseCode ? If not, please add them. Since I am really wondering how this actually works technically since score::Result requires users to implement a free function named MakeError for their custom error code types. And we do not provide any such method, if I have seen correctly.
There was a problem hiding this comment.
You are right... I have added the MakeError implementation for NegativeResponseCode along with the UDS error domain and Unit tests covering score::MakeUnexpected(NegativeResponseCode) & Result error handling are now included in the latest commit...
| "RangedNrc upper bound exceeds NegativeResponseCode range"); | ||
| static_assert(kRangeMin >= static_cast<std::uint8_t>(NegativeResponseCode::GeneralReject), | ||
| "RangedNrc lower bound is below NegativeResponseCode range"); | ||
| return static_cast<NegativeResponseCode>(value_); |
There was a problem hiding this comment.
wouldn't
| return static_cast<NegativeResponseCode>(value_); | |
| return NegativeResponseCode{value_}; |
also work? it's the more clean approach
|
|
||
| /// Result type: either a value T or a UDS NegativeResponseCode. | ||
| template <typename T> | ||
| using Result = score::Result<T>; |
There was a problem hiding this comment.
I see. But do we have any unit tests which verify that error construction actually works in conjunction with uds::NegativeResponseCode ? If not, please add them. Since I am really wondering how this actually works technically since score::Result requires users to implement a free function named MakeError for their custom error code types. And we do not provide any such method, if I have seen correctly.
| public: | ||
| /// Read raw bytes for the data identifier in a fast and non-blocking manner. | ||
| /// @return Result<ByteVector> on success, NegativeResponseCode on failure. | ||
| [[nodiscard]] virtual Result<ByteVector> Read() = 0; |
There was a problem hiding this comment.
@MonikaKumari26 I guess it would be a good idea to always pass Metadata also to all our simplified synchronous APIs so that users have to choice to either consume it or just ignore it, what do you think?
| [[nodiscard]] virtual Result<ByteVector> Read() = 0; | |
| [[nodiscard]] virtual Result<ByteVector> Read(const MetaData& meta_data) = 0; |
There was a problem hiding this comment.
Makes total sense!! I have updated all simplified APIs (ReadDataByIdentifier, WriteDataByIdentifier, RoutineControl, and GenericService) and their corresponding mocks to pass MetaData through...
10037d1 to
2c9f056
Compare
stmuench
left a comment
stmuench
left a comment
There was a problem hiding this comment.
btw, is there any CI job for performing clang-format checks in the inc_diagnostics repo?
| const MetaData& meta_data, | ||
| score::cpp::stop_token /*stop_token*/) final |
There was a problem hiding this comment.
formatting seems to be misaligned
|
|
||
| const auto raw_val = static_cast<std::uint8_t>(code); | ||
|
|
||
| switch (static_cast<NegativeResponseCode>(raw_val)) |
There was a problem hiding this comment.
wo do we need two of such large if statements? from my POV, these can and shall be merged into just one!
6 participants
Description
This PR introduces a C++ implementation of the diagnostics API, mirroring the design and implementation provided in the Rust version.
References