Create hal-device module #471
Conversation
📝 WalkthroughWalkthroughAdds a new hal-device module (sources, headers, license, README) with both ESP-IDF and non-ESP build paths and registers/links it into the build. Introduces C and C++ HAL APIs (hal_device_get_type, hal_device_for_each_of_type, hal_device_get_device/set_device), KernelDeviceHolder and kernel-side register/deregister/find utilities, and a hal_device_module instance. Adds public driver lifecycle APIs (driver_add/driver_remove/driver_construct_add/driver_remove_destruct) and updates call sites. Also normalizes many include paths, narrows several createDevices() return types to std::vector<std::shared_ptrtt::hal::Device>, renames some module identifiers, changes non-ESP crash handling to exit(1), and tweaks file-open mode. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
Modules/hal-device/Include/tactility/drivers/hal_device.h (1)
8-22: Remove misleadingextern "C"wrapper—this header is C++-only in practice.The
enum classsyntax and lack of<stdbool.h>make this header incompatible with C compilation. However, C code in this project usesTactilityC/Include/tt_hal_device.h(a proper C wrapper) instead of including this file directly. Since this header is only included by C++ translation units (.cppfiles andhal_device.hpp), theextern "C"wrapper is misleading and should be removed. If you intend to support C compatibility in the future, either use the separatett_hal_device.hpattern or make this header truly C-compatible.Tactility/Include/Tactility/hal/gps/GpsDevice.h (1)
113-113: Pre-existing: Parameter type mismatch inunsubscribeRmc.The method signature takes
GgaSubscriptionIdbut should likely takeRmcSubscriptionIdfor type consistency with the RMC subscription pattern. This appears to be a pre-existing issue, not introduced by this PR.♻️ Suggested fix
- void unsubscribeRmc(GgaSubscriptionId subscriptionId) { + void unsubscribeRmc(RmcSubscriptionId subscriptionId) {Tactility/Include/Tactility/hal/power/PowerDevice.h (1)
47-47: Minor: Namespace closing comment is inaccurate.The comment says
namespace ttbut the actual namespace being closed istt::hal::power.📝 Suggested fix
-} // namespace tt +} // namespace tt::hal::powerTactilityC/Include/tt_hal_device.h (1)
10-30: Consider a backward-compat alias for the public C API.
RenamingDeviceType→TtDeviceTypebreaks external users; a typedef alias eases migration while keeping the new name.Suggested compatibility alias
typedef enum { DEVICE_TYPE_I2C, DEVICE_TYPE_DISPLAY, DEVICE_TYPE_TOUCH, DEVICE_TYPE_SDCARD, DEVICE_TYPE_KEYBOARD, DEVICE_TYPE_POWER, DEVICE_TYPE_GPS } TtDeviceType; + +/* Backward-compat alias (consider deprecating in a future release) */ +typedef TtDeviceType DeviceType;Modules/hal-device/Include/tactility/hal/Device.h (1)
35-41: MakeKernelDeviceHolder::nameimmutable (or update device->name on change).
device->namepoints atname.c_str(); ifnameis later modified, the pointer can become stale.One possible fix
- struct KernelDeviceHolder { - std::string name = {}; + struct KernelDeviceHolder { + const std::string name; std::shared_ptr<KernelDevice> device = std::make_shared<KernelDevice>(); - explicit KernelDeviceHolder(std::string name) : name(name) { + explicit KernelDeviceHolder(std::string name) : name(std::move(name)) { device->name = this->name.c_str(); } };
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
Modules/hal-device/Source/module.cpp (1)
17-22: Consider using driver_remove_destruct for consistency.
This mirrors other modules and keeps lifecycle handling uniform.♻️ Suggested change
- check(driver_remove(&hal_device_driver) == ERROR_NONE); - check(driver_destruct(&hal_device_driver) == ERROR_NONE); + check(driver_remove_destruct(&hal_device_driver) == ERROR_NONE);Modules/hal-device/Source/hal/Device.cpp (2)
6-9: Add explicit standard headers for used C++ features.This file uses
std::format, ranges/views,std::vector, andstd::back_inserter, but doesn’t include<format>,<ranges>,<vector>, or<iterator>. Relying on transitive includes is brittle.Proposed include additions
`#include` <Tactility/Logger.h> `#include` <Tactility/RecursiveMutex.h> `#include` <algorithm> +`#include` <format> +`#include` <ranges> +`#include` <vector> +`#include` <iterator>Also applies to: 70-78
91-103: Use iterators instead offront()/empty()on filter views for clearer intent.While
std::views::filterover astd::vectordoes exposefront()andempty()in C++20 (since vector is a forward_range), using iterators makes the logic more explicit and maintainable:Iterator-based approach
std::shared_ptr<Device> _Nullable findDevice(const std::function<bool(const std::shared_ptr<Device>&)>& filterFunction) { auto scoped_mutex = mutex.asScopedLock(); scoped_mutex.lock(); auto result_set = getDevices() | std::views::filter([&filterFunction](auto& device) { return filterFunction(device); }); - if (!result_set.empty()) { - return result_set.front(); - } else { - return nullptr; - } + auto it = result_set.begin(); + return (it != result_set.end()) ? *it : nullptr; }bool hasDevice(Device::Type type) { auto scoped_mutex = mutex.asScopedLock(); scoped_mutex.lock(); auto result_set = getDevices() | std::views::filter([&type](auto& device) { return device->getType() == type; }); - return !result_set.empty(); + return result_set.begin() != result_set.end(); }Also applies to: 134-140
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Modules/hal-device/Include/tactility/hal/Device.h (1)
114-114: Unnecessarystd::movemay inhibit NRVO.When returning a local variable, the compiler automatically applies move semantics or NRVO (Named Return Value Optimization). Explicit
std::movecan prevent NRVO.♻️ Suggested fix
- return std::move(result); + return result;
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/Tactility/Main.cpp (1)
50-52:⚠️ Potential issue | 🟡 Minor
vTaskDelete()aftervTaskEndScheduler()is unreachable code.On the FreeRTOS simulator port used here,
vTaskEndScheduler()terminates the process viaexit(0)and does not return. The subsequentvTaskDelete(nullptr)call is unreachable dead code and should be removed.
Summary by CodeRabbit
New Features
Improvements
API Changes
Chores
Fixes
✏️ Tip: You can customize this high-level summary in your review settings.