Add module function definitions #8
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,7 @@ | ||
| FIND_PACKAGE(CMLIB REQUIRED COMPONENTS CMCONF) | ||
|
|
||
| CMCONF_INIT_SYSTEM(FLEET_PROTOCOL) | ||
|
|
||
| SET(STORAGE_LIST DEP) | ||
|
|
||
| SET(STORAGE_LIST_DEP "https://github.com/bacpack-system/package-tracker.git") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| #pragma once | ||
|
|
||
| #include <bringauto/fleet_protocol/cxx/ConvertibleBuffer.hpp> | ||
| #include <bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp> | ||
|
|
||
| #include <bringauto/async_function_execution/AsyncFunctionExecutor.hpp> | ||
|
|
||
|
|
||
| namespace bringauto::fleet_protocol::cxx { | ||
|
Member
There was a problem hiding this comment. please move it to different namesapace. not cx. |
||
|
|
||
| inline static const async_function_execution::FunctionDefinition getModuleNumberAsync { | ||
| async_function_execution::FunctionId { 0 }, | ||
| async_function_execution::Return { int {} }, | ||
| async_function_execution::Arguments {} | ||
| }; | ||
|
|
||
| inline static const async_function_execution::FunctionDefinition isDeviceTypeSupportedAsync { | ||
| async_function_execution::FunctionId { 1 }, | ||
| async_function_execution::Return { int {} }, | ||
| async_function_execution::Arguments { uint32_t {} } | ||
| }; | ||
|
|
||
| inline static const async_function_execution::FunctionDefinition sendStatusConditionAsync { | ||
| async_function_execution::FunctionId { 2 }, | ||
| async_function_execution::Return { int {} }, | ||
| async_function_execution::Arguments { ConvertibleBuffer {}, ConvertibleBuffer {}, uint32_t {} } | ||
| }; | ||
|
|
||
| inline static const async_function_execution::FunctionDefinition generateCommandAsync { | ||
| async_function_execution::FunctionId { 3 }, | ||
| async_function_execution::Return { ConvertibleBufferReturn {} }, | ||
| async_function_execution::Arguments { ConvertibleBuffer {}, ConvertibleBuffer {}, ConvertibleBuffer {}, uint32_t {} } | ||
| }; | ||
|
|
||
| inline static const async_function_execution::FunctionDefinition aggregateStatusAsync { | ||
| async_function_execution::FunctionId { 4 }, | ||
| async_function_execution::Return { ConvertibleBufferReturn {} }, | ||
| async_function_execution::Arguments { ConvertibleBuffer {}, ConvertibleBuffer {}, uint32_t {} } | ||
| }; | ||
|
|
||
| inline static const async_function_execution::FunctionDefinition aggregateErrorAsync { | ||
| async_function_execution::FunctionId { 5 }, | ||
| async_function_execution::Return { ConvertibleBufferReturn {} }, | ||
| async_function_execution::Arguments { ConvertibleBuffer {}, ConvertibleBuffer {}, uint32_t {} } | ||
| }; | ||
|
|
||
| inline static const async_function_execution::FunctionDefinition generateFirstCommandAsync { | ||
| async_function_execution::FunctionId { 6 }, | ||
| async_function_execution::Return { ConvertibleBufferReturn {} }, | ||
| async_function_execution::Arguments { uint32_t {} } | ||
| }; | ||
|
|
||
| inline static const async_function_execution::FunctionDefinition statusDataValidAsync { | ||
| async_function_execution::FunctionId { 7 }, | ||
| async_function_execution::Return { int {} }, | ||
| async_function_execution::Arguments { ConvertibleBuffer {}, uint32_t {} } | ||
| }; | ||
|
|
||
| inline static const async_function_execution::FunctionDefinition commandDataValidAsync { | ||
| async_function_execution::FunctionId { 8 }, | ||
| async_function_execution::Return { int {} }, | ||
| async_function_execution::Arguments { ConvertibleBuffer {}, uint32_t {} } | ||
| }; | ||
|
|
||
| /** | ||
| * @brief Type alias for an AsyncFunctionExecutor specialized with module-related functions. | ||
| */ | ||
| using ModuleFunctionExecutor = async_function_execution::AsyncFunctionExecutor< | ||
| decltype(getModuleNumberAsync), | ||
| decltype(isDeviceTypeSupportedAsync), | ||
| decltype(sendStatusConditionAsync), | ||
| decltype(generateCommandAsync), | ||
| decltype(aggregateStatusAsync), | ||
| decltype(aggregateErrorAsync), | ||
| decltype(generateFirstCommandAsync), | ||
| decltype(statusDataValidAsync), | ||
| decltype(commandDataValidAsync) | ||
| >; | ||
|
|
||
| /** | ||
| * @brief FunctionList containing all module-related function definitions. | ||
| */ | ||
| inline static const async_function_execution::FunctionList< | ||
| decltype(getModuleNumberAsync), | ||
| decltype(isDeviceTypeSupportedAsync), | ||
| decltype(sendStatusConditionAsync), | ||
| decltype(generateCommandAsync), | ||
| decltype(aggregateStatusAsync), | ||
| decltype(aggregateErrorAsync), | ||
| decltype(generateFirstCommandAsync), | ||
| decltype(statusDataValidAsync), | ||
| decltype(commandDataValidAsync) | ||
| > moduleFunctionList { | ||
| getModuleNumberAsync, | ||
| isDeviceTypeSupportedAsync, | ||
| sendStatusConditionAsync, | ||
| generateCommandAsync, | ||
| aggregateStatusAsync, | ||
| aggregateErrorAsync, | ||
| generateFirstCommandAsync, | ||
| statusDataValidAsync, | ||
| commandDataValidAsync | ||
| }; | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| #pragma once | ||
|
|
||
| #include <fleet_protocol/common_headers/device_management.h> | ||
|
|
||
| #include <cstdint> | ||
| #include <span> | ||
|
|
||
|
|
||
|
|
||
| namespace bringauto::fleet_protocol::cxx { | ||
|
|
||
| /** | ||
| * @brief ConvertibleBuffer is a helper class to convert a fleet_protocol 'buffer' struct | ||
| * to/from a byte span for serialization/deserialization. | ||
| */ | ||
| struct ConvertibleBuffer final { | ||
| struct ::buffer buffer {}; | ||
| ConvertibleBuffer() = default; | ||
| ConvertibleBuffer(struct ::buffer buff) : buffer(buff) {} | ||
|
|
||
| std::span<const uint8_t> serialize() const { | ||
| return std::span {reinterpret_cast<const uint8_t *>(buffer.data), buffer.size_in_bytes}; | ||
| } | ||
| void deserialize(std::span<const uint8_t> bytes) { | ||
| auto size = bytes.size(); | ||
| buffer.data = new uint8_t[size]; | ||
| buffer.size_in_bytes = size; | ||
| std::memcpy(buffer.data, bytes.data(), size); | ||
| } | ||
|
Comment on lines
3
to
29
There was a problem hiding this comment. 🧩 Analysis chainClarify shallow, non-owning
To sanity-check usage and ensure expectations around ownership are clear, you can locate all call sites: 🏁 Script executed: #!/bin/bash
rg -n "ConvertibleBuffer" include testLength of output: 2829 I need to gather more information. Let me search for actual Now I'll search the repository directly for the necessary information: Based on my analysis of the code and the learnings from the module-gateway project, I can now generate the rewritten review comment: Add documentation for shallow, non-owning The
For the include path at line 3 ( 🏁 Script executed: #!/bin/bash
# Search for deserialize method calls
rg -n "\.deserialize\(" --type cpp include test src 2>/dev/null | head -20Length of output: 0 🏁 Script executed: #!/bin/bash
# Find device_management.h file
fd -t f "device_management\.h" 2>/dev/nullLength of output: 0 🏁 Script executed: #!/bin/bash
# Check for CMakeLists or include_directories configuration
fd -t f "CMakeLists\.txt" | head -5Length of output: 76 🏁 Script executed: #!/bin/bash
# Look for the buffer struct definition
rg -n "struct.*buffer" --type c --type h -A 5 include | head -50Length of output: 1709 🏁 Script executed: #!/bin/bash
# Check if there's a fleet_protocol subdirectory
find . -type d -name "fleet_protocol" 2>/dev/null | head -10Length of output: 68 I need to check the CMakeLists configuration to understand the include path setup and where device_management comes from: 🏁 Script executed: #!/bin/bash
# Check main CMakeLists.txt for include configuration
cat CMakeLists.txt | head -80Length of output: 2559 🏁 Script executed: #!/bin/bash
# Search for device_management references in build files
rg -i "device_management" --type cmake --type txt 2>/dev/nullLength of output: 0 🏁 Script executed: #!/bin/bash
# Look for external dependencies or include_directories declarations
rg -n "include_directories|find_package|add_subdirectory" CMakeLists.txtLength of output: 0 🏁 Script executed: #!/bin/bash
# Check if fleet_protocol is an external dependency or package
rg -n "fleet_protocol\|fleet-protocol" CMakeLists.txtLength of output: 0 🏁 Script executed: #!/bin/bash
# Search for common_headers directory anywhere in the repo
find . -type d -name "common_headers" 2>/dev/nullLength of output: 0 🏁 Script executed: #!/bin/bash
# Look for buffer struct definition in any header file
rg -n "^\s*struct.*buffer.*{" --type h includeLength of output: 105 🏁 Script executed: #!/bin/bash
# Check for pkg-config or cmake finds related to fleet-protocol
rg -n "pkg_check|find_package|PkgConfig" CMakeLists.txtLength of output: 0 🏁 Script executed: #!/bin/bash
# Examine project structure for dependencies or installed files
ls -la | grep -E "\.cmake|\.pc|conanfile|requirements"Length of output: 67 🏁 Script executed: #!/bin/bash
# Check if the include path exists but just with different structure
find . -path "*fleet_protocol*" -name "*.h" 2>/dev/null | head -20Length of output: 0 🏁 Script executed: #!/bin/bash
# Search all headers in include directory to see what's included
fd -e h . include/bringauto/fleet_protocol/cxx | xargs rg -l "device_management"Length of output: 173 🏁 Script executed: #!/bin/bash
# Check what headers are in the common_headers or device_management areas if they exist
find . -path "*fleet_protocol*common*" -o -path "*device_management*" 2>/dev/nullLength of output: 0 Let me verify the external package structure one more time: 🏁 Script executed: #!/bin/bash
# Check DeviceID.hpp to see how it includes device_management
cat include/bringauto/fleet_protocol/cxx/DeviceID.hpp | head -20Length of output: 483 🏁 Script executed: #!/bin/bash
# Look for any documentation or comments about the external dependency
rg -n "fleet-protocol-interface\|common-headers" --type cmakeLength of output: 0 🏁 Script executed: #!/bin/bash
# Check if there's a conan file or requirements
find . -name "conanfile*" -o -name "requirements.txt" -o -name ".conan*" 2>/dev/nullLength of output: 0 🏁 Script executed: #!/bin/bash
# Check the Dependencies.cmake to understand external packages
cat cmake/Dependencies.cmake 2>/dev/null | head -50Length of output: 314 Add documentation for shallow, non-owning The
The include path 🧰 Tools🪛 Clang (14.0.6)[error] 3-3: 'fleet_protocol/common_headers/device_management.h' file not found (clang-diagnostic-error) 🤖 Prompt for AI Agents |
||
| }; | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| #pragma once | ||
|
|
||
| #include <fleet_protocol/common_headers/device_management.h> | ||
|
|
||
| #include <cstdint> | ||
| #include <cstring> | ||
| #include <span> | ||
|
|
||
|
|
||
|
|
||
| namespace bringauto::fleet_protocol::cxx { | ||
|
|
||
| /** | ||
| * @brief ConvertibleBufferReturn is a helper class to convert a fleet_protocol 'buffer' struct | ||
| * and a return code to/from a byte span for serialization/deserialization. | ||
| * The first 4 bytes represent the return code (int), followed by the buffer data. | ||
| */ | ||
| struct ConvertibleBufferReturn final { | ||
| int returnCode {}; | ||
| struct ::buffer buffer {}; | ||
| ConvertibleBufferReturn() = default; | ||
| ConvertibleBufferReturn(int code, struct ::buffer buff) : returnCode(code), buffer(buff) {} | ||
|
|
||
| std::span<const uint8_t> serialize() const { | ||
| size_t total_size = sizeof(int) + buffer.size_in_bytes; | ||
| uint8_t* data = new uint8_t[total_size]; | ||
| std::memcpy(data, &returnCode, sizeof(int)); | ||
| std::memcpy(data + sizeof(int), buffer.data, buffer.size_in_bytes); | ||
| return {data, total_size}; | ||
| } | ||
| void deserialize(std::span<const uint8_t> bytes) { | ||
| auto size = bytes.size(); | ||
| if (size < sizeof(int)) return; | ||
| std::memcpy(&returnCode, bytes.data(), sizeof(int)); | ||
| size -= sizeof(int); | ||
| buffer.data = new uint8_t[size]; | ||
| buffer.size_in_bytes = size; | ||
| std::memcpy(buffer.data, bytes.data() + sizeof(int), size); | ||
| } | ||
|
Comment on lines
18
to
39
There was a problem hiding this comment. 🧩 Analysis chainFix ownership/lifetime in
A safer pattern is:
-struct ConvertibleBufferReturn final {
- int returnCode {};
- struct ::buffer buffer {};
+struct ConvertibleBufferReturn final {
+ int returnCode {};
+ struct ::buffer buffer {};
+ mutable std::vector<uint8_t> serializedData{};
@@
- std::span<const uint8_t> serialize() const {
- size_t total_size = sizeof(int) + buffer.size_in_bytes;
- uint8_t* data = new uint8_t[total_size];
- std::memcpy(data, &returnCode, sizeof(int));
- std::memcpy(data + sizeof(int), buffer.data, buffer.size_in_bytes);
- return {data, total_size};
- }
- void deserialize(std::span<const uint8_t> bytes) {
- auto size = bytes.size();
- if (size < sizeof(int)) return;
- std::memcpy(&returnCode, bytes.data(), sizeof(int));
- size -= sizeof(int);
- buffer.data = new uint8_t[size];
- buffer.size_in_bytes = size;
- std::memcpy(buffer.data, bytes.data() + sizeof(int), size);
- buffer.size_in_bytes = size;
- }
+ std::span<const uint8_t> serialize() const {
+ const auto headerSize = sizeof(returnCode);
+ const auto payloadSize = buffer.size_in_bytes;
+ serializedData.resize(headerSize + payloadSize);
+ std::memcpy(serializedData.data(), &returnCode, headerSize);
+ std::memcpy(serializedData.data() + headerSize, buffer.data, payloadSize);
+ return {serializedData.data(), serializedData.size()};
+ }
+
+ void deserialize(std::span<const uint8_t> bytes) {
+ const auto size = bytes.size();
+ const auto headerSize = sizeof(returnCode);
+ if (size < headerSize) {
+ return;
+ }
+
+ std::memcpy(&returnCode, bytes.data(), headerSize);
+ const auto payloadSize = size - headerSize;
+ buffer.data = const_cast<uint8_t *>(bytes.data() + headerSize);
+ buffer.size_in_bytes = payloadSize;
+ }This eliminates the leaks and matches the shallow-copy semantics already used for I'll verify the review comment by examining the codebase structure and comparing the current implementation against the pattern used elsewhere. 🏁 Script executed: #!/bin/bash
# 1. Locate the file and confirm it exists
fd -t f ConvertibleBufferReturn.hpp
# 2. Find ConvertibleBuffer to compare patterns
fd -t f ConvertibleBuffer.hpp
# 3. Search for ::buffer struct definition
rg -n "struct\s+::?buffer" --type=cpp --type=hpp -A 5Length of output: 151 🏁 Script executed: #!/bin/bash
# 4. Check for destructors or cleanup in ConvertibleBufferReturn
rg -n "ConvertibleBufferReturn" --type=cpp --type=hpp -C 3
# 5. Look for usages of serialize() to understand lifetime
rg -n "\.serialize\(\)" --type=cpp --type=hpp -C 2Length of output: 55 🏁 Script executed: #!/bin/bash
# 6. Examine ConvertibleBuffer::deserialize implementation to confirm shallow aliasing
ast-grep --pattern $'struct ConvertibleBuffer {
$$$
deserialize($_) {
$$$
}
$$$
}'Length of output: 0 🏁 Script executed: #!/bin/bash
# View the two key files directly
echo "=== ConvertibleBufferReturn.hpp ==="
cat include/bringauto/fleet_protocol/cxx/ConvertibleBufferReturn.hpp
echo ""
echo "=== ConvertibleBuffer.hpp ==="
cat include/bringauto/fleet_protocol/cxx/ConvertibleBuffer.hppLength of output: 2142 🏁 Script executed: #!/bin/bash
# Search for deserialize patterns without type restrictions
echo "=== Usages of deserialize ==="
rg -n "deserialize" -C 3
echo ""
echo "=== Struct ::buffer definition ==="
rg -n "struct.*buffer" -C 2Length of output: 5393 🏁 Script executed: #!/bin/bash
# Check for usage of serialize and any cleanup/deleter patterns
echo "=== Serialize usages ==="
rg -n "serialize\(\)" -C 2
echo ""
echo "=== Check for delete or std::unique_ptr around buffer ==="
rg -n "delete.*buffer|std::unique_ptr.*buffer" -C 2Length of output: 1653 Fix memory leaks in The review comment correctly identifies critical memory leaks:
Verified file state matches the snippet exactly. The implementation also diverges from the existing The proposed fix—internal owning 🤖 Prompt for AI Agents |
||
| }; | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #include <bringauto/fleet_protocol/cxx/AsyncModuleFunctionDefinitions.hpp> | ||
|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| using namespace bringauto::fleet_protocol::cxx; | ||
|
|
||
|
|
||
| TEST(AsyncModuleFunctionDefinitions_tests, FunctionDefinitionsExist) { | ||
| EXPECT_EQ(getModuleNumberAsync.id.value, 0); | ||
| EXPECT_EQ(isDeviceTypeSupportedAsync.id.value, 1); | ||
| EXPECT_EQ(sendStatusConditionAsync.id.value, 2); | ||
| EXPECT_EQ(generateCommandAsync.id.value, 3); | ||
| EXPECT_EQ(aggregateStatusAsync.id.value, 4); | ||
| EXPECT_EQ(aggregateErrorAsync.id.value, 5); | ||
| EXPECT_EQ(generateFirstCommandAsync.id.value, 6); | ||
| EXPECT_EQ(statusDataValidAsync.id.value, 7); | ||
| EXPECT_EQ(commandDataValidAsync.id.value, 8); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Ensure BA package names match the CMake
find_packageusageAdding
aeron v1.48.6andasync-function-execution v1.0.0here looks consistent with the PR description, but please confirm that:aeron,async-function-execution) correspond to the actual package IDs in your package tracker.find_package(e.g.,async-function-execution-sharedin CMakeLists, if that’s what you use), so CMLIB resolves the same artifact.You can grep for how these libraries are consumed:
🏁 Script executed:
Length of output: 392
Fix package name mismatch for async-function-execution-shared
The verification reveals a critical inconsistency:
cmake/Dependencies.cmake:4definesBA_PACKAGE_LIBRARY(async-function-execution v1.0.0), butCMakeLists.txt:44callsFIND_PACKAGE(async-function-execution-shared 1.0.0 REQUIRED).The BA package name must match what
find_packageresolves. Change line 4 incmake/Dependencies.cmakefromasync-function-executiontoasync-function-execution-shared. The aeron entry (line 3) is correctly named.🤖 Prompt for AI Agents