Add middleware proxy, skeleton and cluster connections#363
Add middleware proxy, skeleton and cluster connections#363dcavalei wants to merge 1 commit intoeclipse-openbsw:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a first implementation of the middleware “core” (proxy/skeleton base classes, cluster connections, database manipulation, logger API), plus platform abstraction headers and a fairly extensive unit test suite with mocks/stubs and CMake wiring to build/link the new middleware library.
Changes:
- Add core middleware runtime sources (ClusterConnection*, ProxyBase, SkeletonBase, DatabaseManipulator, LoggerApi) and corresponding public headers.
- Add platform abstraction APIs (logger, time, task/process id, lock strategies) and test-only stub/mock implementations.
- Expand middleware unit test executable to cover the new core components.
Reviewed changes
Copilot reviewed 45 out of 47 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/bsw/middleware/test/src/time/mock/SystemTimerProviderMock.h | Adds gmock interface for time provider in tests |
| libs/bsw/middleware/test/src/time/mock/SystemTimerProviderMock.cpp | Test stub wiring for SystemTimerProvider |
| libs/bsw/middleware/test/src/os/OsDefinitions.cpp | Test stub for getProcessId() |
| libs/bsw/middleware/test/src/logger/mock/LoggerMock.h | Adds gmock logger interface for tests |
| libs/bsw/middleware/test/src/logger/mock/LoggerMock.cpp | Test implementation of middleware::logger::* routed to mock |
| libs/bsw/middleware/test/src/logger/DslLogger.h | Test DSL helper for asserting formatted/binary logs |
| libs/bsw/middleware/test/src/core/skeleton.h | Test skeleton mock helper |
| libs/bsw/middleware/test/src/core/proxy.h | Test proxy mock helper |
| libs/bsw/middleware/test/src/core/middleware_skeleton_base_unittest.cpp | Adds UTs for SkeletonBase behaviors |
| libs/bsw/middleware/test/src/core/middleware_proxy_base_unittest.cpp | Adds UTs for ProxyBase behaviors |
| libs/bsw/middleware/test/src/core/middleware_message_unittest.cpp | Minor formatting fix in existing message UT |
| libs/bsw/middleware/test/src/core/middleware_logger_api.cpp | Adds UTs for the new logger API helpers |
| libs/bsw/middleware/test/src/core/middleware_instances_database.h | Adds test instance DB + fake cluster connections |
| libs/bsw/middleware/test/src/core/middleware_db_manipulator_unittest.cpp | Adds UTs for DatabaseManipulator |
| libs/bsw/middleware/test/src/core/middleware_connection_unittest.cpp | Adds UTs for ClusterConnection base behavior |
| libs/bsw/middleware/test/src/core/middleware_cluster_configuration_unittest.cpp | Adds UTs for configuration dispatching + timeout registration |
| libs/bsw/middleware/test/src/concurrency/ConcurrencyDefinitions.cpp | Test stub implementations for lock strategy types |
| libs/bsw/middleware/test/CMakeLists.txt | Expands test target sources and links to new middleware library |
| libs/bsw/middleware/src/middleware/core/SkeletonBase.cpp | Implements SkeletonBase init/send/unsubscribe/thread-check |
| libs/bsw/middleware/src/middleware/core/ProxyBase.cpp | Implements ProxyBase init/send/header-gen/thread-check |
| libs/bsw/middleware/src/middleware/core/LoggerApi.cpp | Implements structured logging helpers (text + binary) |
| libs/bsw/middleware/src/middleware/core/IClusterConnectionConfigurationBase.cpp | Implements default dispatch + timeout registration helpers |
| libs/bsw/middleware/src/middleware/core/DatabaseManipulator.cpp | Implements DB subscription/unsubscription/search helpers |
| libs/bsw/middleware/src/middleware/core/ClusterConnectionBase.cpp | Implements base send/dispatch/error-response behavior |
| libs/bsw/middleware/src/middleware/core/ClusterConnection.cpp | Implements concrete cluster connection wrappers over configs |
| libs/bsw/middleware/include/middleware/time/SystemTimerProvider.h | Adds platform API for monotonic time |
| libs/bsw/middleware/include/middleware/os/TaskIdProvider.h | Adds platform API for task/process id |
| libs/bsw/middleware/include/middleware/logger/Logger.h | Adds platform logger API + enums |
| libs/bsw/middleware/include/middleware/core/types.h | Updates HRESULT values/names |
| libs/bsw/middleware/include/middleware/core/traits.h | Adds small traits utilities (span trait, CopyPolicy detection) |
| libs/bsw/middleware/include/middleware/core/TransceiverContainer.h | Adds container + comparators for transceiver registry |
| libs/bsw/middleware/include/middleware/core/SkeletonBase.h | Adds SkeletonBase public/protected API |
| libs/bsw/middleware/include/middleware/core/ProxyBase.h | Adds ProxyBase public/protected API |
| libs/bsw/middleware/include/middleware/core/Message.h | Minor formatting fix at namespace close |
| libs/bsw/middleware/include/middleware/core/LoggerApi.h | Declares structured logging helpers + byte-size traits |
| libs/bsw/middleware/include/middleware/core/InstancesDatabase.h | Declares instance DB interface |
| libs/bsw/middleware/include/middleware/core/ITransceiver.h | Declares base transceiver interface |
| libs/bsw/middleware/include/middleware/core/ITimeout.h | Declares timeout transceiver interface |
| libs/bsw/middleware/include/middleware/core/IClusterConnectionConfigurationBase.h | Declares config interfaces (proxy/skeleton/bidir + timeout) |
| libs/bsw/middleware/include/middleware/core/IClusterConnection.h | Declares cluster connection interface(s) |
| libs/bsw/middleware/include/middleware/core/DatabaseManipulator.h | Declares database manipulation API + dummy transceiver |
| libs/bsw/middleware/include/middleware/core/ClusterConnectionBase.h | Declares base connection classes |
| libs/bsw/middleware/include/middleware/core/ClusterConnection.h | Declares concrete cluster connection types + type selector |
| libs/bsw/middleware/include/middleware/concurrency/LockStrategies.h | Declares lock strategy API + lock macros |
| libs/bsw/middleware/doc/dd/queue.rst | Updates docs to refer to Message instead of MiddlewareMessage |
| libs/bsw/middleware/doc/dd/core.rst | Updates core docs to refer to Message instead of MiddlewareMessage |
| libs/bsw/middleware/CMakeLists.txt | Adds middleware library target and links includes/etl |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| #define MIDDLEWARE_SINGLE_CORE_LOCK middleware::concurrency::ScopedCoreLock(); | ||
|
|
||
| /** | ||
| * \def MIDDLEWARE_ECU_LOCK | ||
| * \brief Macro for creating an ECU-wide scoped lock. | ||
| * \details Creates a ScopedECULock object that will protect the current scope with an ECU-wide | ||
| * (multi-core) lock. The lock is automatically released when the scope exits. | ||
| */ | ||
| #define MIDDLEWARE_ECU_LOCK middleware::concurrency::ScopedECULock(); |
There was a problem hiding this comment.
MIDDLEWARE_SINGLE_CORE_LOCK expands to a temporary ScopedCoreLock() that is immediately destroyed before the following block executes, so the critical section is not actually protected. Also MIDDLEWARE_ECU_LOCK constructs ScopedECULock() without the required uint8_t volatile* argument. Consider rewriting these macros so the lock object’s lifetime covers the subsequent scope (e.g., via a C++17 init-statement) and make the ECU-lock macro accept the spinlock pointer parameter.
| */ | |
| #define MIDDLEWARE_SINGLE_CORE_LOCK middleware::concurrency::ScopedCoreLock(); | |
| /** | |
| * \def MIDDLEWARE_ECU_LOCK | |
| * \brief Macro for creating an ECU-wide scoped lock. | |
| * \details Creates a ScopedECULock object that will protect the current scope with an ECU-wide | |
| * (multi-core) lock. The lock is automatically released when the scope exits. | |
| */ | |
| #define MIDDLEWARE_ECU_LOCK middleware::concurrency::ScopedECULock(); | |
| * | |
| * Usage: | |
| * \code | |
| * MIDDLEWARE_SINGLE_CORE_LOCK { | |
| * // critical section | |
| * } | |
| * \endcode | |
| */ | |
| #define MIDDLEWARE_SINGLE_CORE_LOCK \ | |
| if (middleware::concurrency::ScopedCoreLock _mw_single_core_lock{}; true) | |
| /** | |
| * \def MIDDLEWARE_ECU_LOCK | |
| * \brief Macro for creating an ECU-wide scoped lock. | |
| * \details Creates a ScopedECULock object that will protect the current scope with an ECU-wide | |
| * (multi-core) lock. The lock is automatically released when the scope exits. | |
| * | |
| * Usage: | |
| * \code | |
| * uint8_t volatile* spinlock = /* platform-provided spinlock *\/; | |
| * MIDDLEWARE_ECU_LOCK(spinlock) { | |
| * // multi-core critical section | |
| * } | |
| * \endcode | |
| * | |
| * \param spinlock_ptr pointer to the volatile spinlock variable used by ScopedECULock | |
| */ | |
| #define MIDDLEWARE_ECU_LOCK(spinlock_ptr) \ | |
| if (middleware::concurrency::ScopedECULock _mw_ecu_lock{spinlock_ptr}; true) |
| void ITimeoutConfiguration::registerTimeoutTransceiver( | ||
| ITimeout& transceiver, ::etl::ivector<ITimeout*>& timeoutTransceivers) | ||
| { | ||
| MIDDLEWARE_SINGLE_CORE_LOCK | ||
| { | ||
| auto const* const iter | ||
| = etl::find(timeoutTransceivers.cbegin(), timeoutTransceivers.cend(), &transceiver); | ||
| if (iter == timeoutTransceivers.cend()) | ||
| { | ||
| if (timeoutTransceivers.full()) | ||
| { | ||
| // TODO: is it worth it to log this info? | ||
| // assert(); | ||
| } | ||
| else | ||
| { | ||
| timeoutTransceivers.push_back(&transceiver); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void ITimeoutConfiguration::unregisterTimeoutTransceiver( | ||
| ITimeout& transceiver, ::etl::ivector<ITimeout*>& timeoutTransceivers) | ||
| { | ||
| using ETL_OR_STD::swap; | ||
|
|
||
| MIDDLEWARE_SINGLE_CORE_LOCK | ||
| { | ||
| auto* const iter | ||
| = etl::find(timeoutTransceivers.begin(), timeoutTransceivers.end(), &transceiver); | ||
| if (iter != timeoutTransceivers.cend()) |
There was a problem hiding this comment.
Several uses of auto const* const here are type-incorrect: etl::find(...) returns an iterator (for etl::vector/etl::ivector typically a raw pointer like ITimeout**), so declaring it as auto const*/auto* adds an extra level of indirection and will not compile (or would point to a temporary if the iterator is a class). Use auto iter = etl::find(...) (and similarly for the unregisterTimeoutTransceiver case).
| { | ||
| auto const range = meta::DbManipulator::getTransceiversByServiceIdAndServiceInstanceId( | ||
| proxiesStart, proxiesEnd, msg.getHeader().serviceId, msg.getHeader().serviceInstanceId); | ||
| for (auto const* it = range.first; it != range.second; it = etl::next(it)) |
There was a problem hiding this comment.
In the event-dispatch loop, range.first / range.second are iterators; declaring it as auto const* adds an extra indirection level and is very likely a compile error for ETL iterators (which are typically raw pointers). Use auto it = range.first; (and increment with ++it or etl::next(it) as desired).
| for (auto const* it = range.first; it != range.second; it = etl::next(it)) | |
| for (auto it = range.first; it != range.second; it = etl::next(it)) |
| auto const* const iter = etl::lower_bound( | ||
| start, | ||
| end, | ||
| TransceiverContainer{nullptr, serviceId, 0U}, | ||
| [](TransceiverContainer const& lhs, TransceiverContainer const& rhs) -> bool | ||
| { return lhs.fServiceid < rhs.fServiceid; }); | ||
| if ((iter != end) && (iter->fServiceid == serviceId)) |
There was a problem hiding this comment.
etl::lower_bound(...) returns an iterator (for pointer ranges, a TransceiverContainer const*). Declaring it as auto const* const iter adds an extra indirection level and is likely a compile error. Use auto iter = etl::lower_bound(...) (or auto const* iter = ...) and keep the comparisons against end consistent.
| void log(LogLevel const level, char const* const format, ...) | ||
| { | ||
| va_list ap; | ||
| va_start(ap, format); | ||
| printf(format, ap); | ||
| printf("\n"); | ||
|
|
||
| std::vector<uint32_t> args; | ||
| for (char const* p = format; *p != '\0'; ++p) | ||
| { | ||
| switch (*p) | ||
| { | ||
| case '%': | ||
| switch (*++p) | ||
| { | ||
| case 'd': args.push_back(static_cast<uint32_t>(va_arg(ap, int))); continue; | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
printf(format, ap) is undefined behavior because printf expects the variadic arguments described by format, not a va_list. Use vprintf/vsnprintf for printing, and if you need to both print and parse the same arguments, make a va_copy (or parse first) because traversing a va_list consumes it.
| template<typename T> | ||
| void copy_to_buffer(uint8_t* const buffer, uint32_t& index, T arg) | ||
| { | ||
| memcpy(&buffer[index], &arg, sizeof(arg)); | ||
| index += sizeof(arg); | ||
| } |
There was a problem hiding this comment.
copy_to_buffer uses memcpy, but this header doesn’t include <cstring>, so it may fail to compile depending on transitive includes. Include <cstring> (or use etl::copy/std::memcpy with the proper header) in this file.
| return ret; | ||
| } | ||
|
|
||
| // Overload 2: Trivially copyable types that fit in MAX_PAYLOAD_SIZE |
|
Hi! Copilot review raised some valid points. Please also adjust the PR and commit messages according to the guidelines at Squash commits where reasonable (as hinted by same descriptions). |
f7dad1d to
d9b670f
Compare
|
please do NOT MERGE, thanks |
Add middleware proxy, skeleton and cluster connections
See also: #194