Create TactilityFreertos subproject #440
Conversation
📝 WalkthroughWalkthroughAdds a new TactilityFreeRtos component providing FreeRTOS wrappers (Thread, Timer, Mutex, RecursiveMutex, Semaphore, MessageQueue, Dispatcher, EventGroup, and freertoscompat headers), new CMake targets and tests. Removes many TactilityCore FreeRTOS wrappers and compatibility headers and deletes core implementations (Thread, Timer, MessageQueue, Semaphore, StreamBuffer, Dispatcher, EventFlag, kernel wrappers). Propagates API changes across the codebase: TAG macros → constexpr, several unlock/release functions change return type to void, default timeouts use kernel::MAX_TICKS, timer and thread APIs/signatures adjusted, and Thread ID types renamed to TaskHandle. Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
Tactility/Source/service/ServiceRegistration.cpp (1)
56-59: Critical: Wrong mutex protects service_instance_map.The function uses
manifest_mutexto protect access toservice_instance_map, but based on the naming convention and usage elsewhere (lines 75-77, 85-87, 116-118), it should useinstance_mutexinstead. This creates a race condition when concurrent access occurs to both maps.🔎 Proposed fix
static std::shared_ptr<ServiceInstance> _Nullable findServiceInstanceById(const std::string& id) { - manifest_mutex.lock(); + instance_mutex.lock(); auto iterator = service_instance_map.find(id); auto service = iterator != service_instance_map.end() ? iterator->second : nullptr; - manifest_mutex.unlock(); + instance_mutex.unlock(); return service; }Note: This issue exists in the unchanged code and is not introduced by this PR, but it's critical enough to address.
TactilityCore/Source/kernel/critical/Critical.cpp (1)
33-41: Critical bug: exit() function uses ENTER_CRITICAL instead of EXIT_CRITICAL.Line 37 incorrectly uses
TT_ENTER_CRITICAL()when it should use the corresponding exit macro. This causes critical section mismanagement, potentially leading to deadlocks or corrupted state. While this is a pre-existing bug (not introduced by this PR), it must be fixed.🔎 Proposed fix
Add the missing macro definition and fix the exit function:
#ifdef ESP_PLATFORM static portMUX_TYPE critical_mutex; #define TT_ENTER_CRITICAL() taskENTER_CRITICAL(&critical_mutex) +#define TT_EXIT_CRITICAL() taskEXIT_CRITICAL(&critical_mutex) #else #define TT_ENTER_CRITICAL() taskENTER_CRITICAL() +#define TT_EXIT_CRITICAL() taskEXIT_CRITICAL() #endifvoid exit(CriticalInfo info) { if (info.fromIsr) { taskEXIT_CRITICAL_FROM_ISR(info.isrm); } else if (info.kernelRunning) { - TT_ENTER_CRITICAL(); + TT_EXIT_CRITICAL(); } else { portENABLE_INTERRUPTS(); } }Tests/TactilityFreeRtos/ThreadTest.cpp (1)
88-99: Fix duplicate test case name.This test case has the same name as the test at Line 44 ("thread id should only be set at when thread is started"), which will cause issues with test frameworks. This appears to be testing the return code functionality.
🔎 Proposed fix
-TEST_CASE("thread id should only be set at when thread is started") { +TEST_CASE("thread should return the correct return code") {TactilityFreeRtos/Include/Tactility/PubSub.h (2)
96-105: Holding mutex during callback invocation creates deadlock risk.The
publish()method holds the lock while invoking callbacks (lines 100-102). If any callback attempts to callsubscribe(),unsubscribe(), orpublish()on the samePubSubinstance, it will deadlock sinceMutexis non-recursive.This contradicts the "Re-entrable" documentation on line 93. Consider copying the callback list under lock, then invoking callbacks without the lock:
🔎 Proposed fix
void publish(DataType data) { - mutex.lock(); - - // Iterate over subscribers - for (auto& it : items) { - it.callback(data); - } - - mutex.unlock(); + mutex.lock(); + // Copy callbacks to allow re-entrancy + auto callbacks = items; + mutex.unlock(); + + // Invoke callbacks without holding the lock + for (auto& it : callbacks) { + it.callback(data); + } }
51-67:subscribe()documentation claims "Re-entrable" but uses non-recursive mutex.If a callback (invoked during
publish()) tries to callsubscribe(), it will deadlock. Either update the documentation to clarify the limitation, or use aRecursiveMutex.Tactility/Source/hal/gps/GpsInit.cpp (1)
106-106: Off-by-one error in buffer position check.The condition
bufferPos == sizeof(buffer) - 1checks for 13 bytes whenCAS_ACK_NACK_MSG_SIZE(14 bytes as per the comment) is expected. This should bebufferPos == sizeof(buffer)orbufferPos >= CAS_ACK_NACK_MSG_SIZE.🔎 Proposed fix
- if (bufferPos == sizeof(buffer) - 1) { + if (bufferPos == sizeof(buffer)) {
🟡 Minor comments (10)
TactilityC/Source/tt_timer.cpp-28-34 (1)
28-34: API split is well-designed and migration is complete.The
tt_timer_resetandtt_timer_reset_with_intervalfunctions are properly implemented and exported. No remaining references to the removedtt_timer_restartfunction exist in the codebase, confirming successful migration.Minor fix needed: Update the documentation comments in the header file (TactilityC/Include/tt_timer.h, lines 43-46 and 51-53). The comments still say "Restart an already started timer" but should say "Reset" to match the new function names.
Tactility/Source/app/gpssettings/GpsSettings.cpp-65-65 (1)
65-65: Timer API pattern is correctly implemented in GpsSettings.cpp, but test code needs updating.The
timer->start()call at line 65 correctly omits the interval parameter since the interval is provided at Timer construction (line 271 withkernel::secondsToTicks(1)). This pattern is consistent across the production codebase.However, test code in
Tests/TactilityFreeRtos/TimerTest.cppstill uses the old API pattern, callingtimer->start(interval_value)with interval parameters. These tests should be updated to calltimer->start()without parameters to align with the new API.Tactility/Source/service/gui/GuiService.cpp-183-184 (1)
183-184: Unused variabletask_handle.The
task_handleis assigned but never used. This appears to be leftover code from a previous implementation.🔎 Proposed fix
- auto task_handle = thread->getTaskHandle(); threadFlags.set(GUI_THREAD_FLAG_EXIT);Tactility/Source/service/gui/GuiService.cpp-196-197 (1)
196-197: Unused variabletask_handle.Same issue as in
onStop()- the variable is assigned but never used.🔎 Proposed fix
void GuiService::requestDraw() { - auto task_handle = thread->getTaskHandle(); threadFlags.set(GUI_THREAD_FLAG_DRAW); }Tactility/Source/service/gui/GuiService.cpp-40-46 (1)
40-46: Variable shadowing: redundant service lookup.Line 42 declares a new
servicevariable that shadows theservicefrom line 28. Since both refer to the same service, the inner lookup is redundant and potentially confusing.🔎 Proposed fix
// Process and dispatch draw call if (flags & GUI_THREAD_FLAG_DRAW) { service->threadFlags.clear(GUI_THREAD_FLAG_DRAW); - auto service = findService(); - if (service != nullptr) { - service->redraw(); - } + service->redraw(); }TactilityFreeRtos/Include/Tactility/Thread.h-114-118 (1)
114-118: Typo in documentation comment.Minor typo: "mjust" should be "must".
🔎 Proposed fix
- /** @warning If thread is running, you mjust call join() first */ + /** @warning If thread is running, you must call join() first */TactilityFreeRtos/README.md-5-7 (1)
5-7: Update the license file path fromLICENSE.mdto../LICENSE.md.The file does not exist in the
TactilityFreeRtos/directory. The actual license is in the repository root.TactilityFreeRtos/Include/Tactility/Semaphore.h-118-119 (1)
118-119:unlock()silently discards failure fromrelease().The
Lockinterface requiresvoid unlock(), butrelease()returns a boolean indicating success/failure. In ISR context,release()can fail (e.g., semaphore already at max count). Silently ignoring this could hide bugs.Consider adding an assertion or logging on failure, consistent with how other Lock implementations handle this.
🔎 Proposed fix
/** Calls release() */ - void unlock() const override { release(); } + void unlock() const override { + bool success = release(); + assert(success); + (void)success; // Suppress unused warning in release builds + }Tests/TactilityFreeRtos/RecursiveMutexTest.cpp-33-38 (1)
33-38: Missing secondunlock()call leaves mutex in locked state.The test locks the mutex twice but only unlocks once. A
RecursiveMutexrequires balanced lock/unlock calls; this leaves the mutex partially locked when the test ends, which could affect other tests or leak resources.🔎 Proposed fix
TEST_CASE("a RecursiveMutex can be locked more than once from the same context") { RecursiveMutex mutex; CHECK_EQ(mutex.lock(0), true); CHECK_EQ(mutex.lock(0), true); mutex.unlock(); + mutex.unlock(); }TactilityFreeRtos/Include/Tactility/MessageQueue.h-119-128 (1)
119-128: Documentation mismatch: reset() returns void but doc says "@return success result".The documentation states
@return success resultbut the function returnsvoid. Either update the return type tobool(like the TactilityCore version) or fix the documentation.🔎 Proposed fix (option 1: fix documentation)
/** * Reset queue * @warning Don't call this from ISR context - * @return success result */ void reset() const {
🧹 Nitpick comments (35)
Tactility/Source/service/screenshot/Screenshot.cpp (1)
15-15: Consider making the TAG type explicit.While
constexpr auto* TAG = "ScreenshotService";works correctly (auto deduces toconst char*), using an explicit type would improve clarity:-constexpr auto* TAG = "ScreenshotService"; +constexpr const char* TAG = "ScreenshotService";Alternatively,
constexpr std::string_view TAG = "ScreenshotService";would also be suitable if used consistently across the codebase.Tactility/Include/Tactility/service/loader/Loader.h (1)
3-11: Include reordering looks good.The reordered includes maintain all necessary dependencies for the class definition. No headers were removed or added, and all type usage (PubSub, RecursiveMutex, DispatcherThread, Bundle, AppInstance, AppContext, Service) is covered.
One minor note: the system include
<memory>is placed after local includes. If your project follows a strict include ordering convention (system headers before local headers), this could be moved to line 3 for consistency.TactilityC/Source/tt_message_queue.cpp (1)
28-30: Simplify the unnecessary void return statement.Line 29 returns a void expression, which is valid C++ but unnecessary. Simplify for clarity:
Proposed change
void tt_message_queue_reset(MessageQueueHandle handle) { - return HANDLE_TO_MESSAGE_QUEUE(handle)->reset(); + HANDLE_TO_MESSAGE_QUEUE(handle)->reset(); }The underlying
MessageQueue::reset()method returnsvoid, making thereturnstatement redundant.Tactility/Source/service/ServiceRegistration.cpp (1)
30-36: Consider using RAII for mutex management.The code uses manual
lock()andunlock()calls throughout. Consider using RAII-based lock guards (e.g.,std::lock_guardor a custom wrapper) to ensure mutexes are always released, even if an exception is thrown.Note: This applies to unchanged code and can be addressed in a future refactoring effort.
Also applies to: 48-51, 75-77, 85-87, 116-118
Tactility/Source/kernel/SystemEvents.cpp (2)
59-71: Consider logging lock acquisition failure.If
mutex.lock(kernel::MAX_TICKS)fails, events are silently dropped without any indication. While lock failures should be rare with an indefinite timeout, adding error logging would aid debugging if synchronization issues occur.🔎 Optional: Add error logging on lock failure
void publishSystemEvent(SystemEvent event) { TT_LOG_I(TAG, "%s", getEventName(event)); if (mutex.lock(kernel::MAX_TICKS)) { for (auto& subscription : subscriptions) { if (subscription.event == event) { subscription.handler(event); } } mutex.unlock(); + } else { + TT_LOG_E(TAG, "Failed to acquire mutex for event %s", getEventName(event)); } }
90-97: Consider logging lock acquisition failure.If
mutex.lock(kernel::MAX_TICKS)fails, the subscription is silently left in place, potentially causing resource leaks or continued event handling. Adding error logging would help diagnose synchronization issues.🔎 Optional: Add error logging on lock failure
void unsubscribeSystemEvent(SystemEventSubscription subscription) { if (mutex.lock(kernel::MAX_TICKS)) { std::erase_if(subscriptions, [subscription](auto& item) { return (item.id == subscription); }); mutex.unlock(); + } else { + TT_LOG_E(TAG, "Failed to acquire mutex for unsubscribing subscription %u", subscription); } }TactilityC/Source/tt_timer.cpp (1)
57-59: Consider renamingtt_timer_set_thread_priorityto align with underlying C++ API.The wrapper calls
setCallbackPriority(), but the C wrapper function retains the namett_timer_set_thread_priority. Renaming tott_timer_set_callback_prioritywould improve clarity and consistency with the underlying implementation. Since the function is not used internally in the codebase, this change would not break internal code, though external users of this C API (if any) would need to update their calls.Tactility/Source/service/statusbar/Statusbar.cpp (2)
268-270: Refactor lambda capture to avoid circular reference.The lambda captures a
shared_ptr<StatusbarService>, creating a circular reference:StatusbarServiceownsupdateTimer(viaunique_ptr), and the timer's callback holds ashared_ptrback to the service. WhileonStop()breaks this cycle, it artificially inflates the reference count during normal operation and could leak ifonStop()is skipped due to an error path.Since the
Timeris owned by the service and destroyed inonStop(), capturing rawthisis safe and cleaner.🔎 Recommended fix
- updateTimer = std::make_unique<Timer>(Timer::Type::Periodic, pdMS_TO_TICKS(1000), [service] { - service->update(); + updateTimer = std::make_unique<Timer>(Timer::Type::Periodic, pdMS_TO_TICKS(1000), [this] { + this->update(); });
140-140: Consider removing unused mutex.The
mutexmember and itslock()/unlock()methods appear unused—no calls are visible in the file. Theupdate()method useslvgl::lock()directly instead. If this mutex is not needed for future functionality, consider removing it to reduce clutter and memory footprint.🔎 Proposed cleanup
- Mutex mutex; std::unique_ptr<Timer> updateTimer; int8_t gps_icon_id;- void lock() const { - mutex.lock(); - } - - void unlock() const { - mutex.unlock(); - } - void updateGpsIcon() {Also applies to: 153-159
Tactility/Source/network/Http.cpp (1)
34-34: Track the TODO comment.The TODO comment indicates that there are missing initializer warnings for the
esp_http_client_config_tstruct. This is a common issue with ESP-IDF structs that have many fields.Do you want me to open a new issue to track this task?
TactilityFreeRtos/Include/Tactility/freertoscompat/PortCompat.h (1)
6-6: Consider suppressing unused parameter warning.The macro parameter
xis unused, which may trigger compiler warnings. Consider casting to void if the parameter must match the ESP platform signature.🔎 Proposed change to suppress unused parameter warning
-#define xPortInIsrContext(x) (false) +#define xPortInIsrContext(x) ((void)(x), false)LICENSE.md (1)
10-10: Minor grammar improvement: hyphenate "Third party".Consider changing "Third party" to "Third-party" for proper compound adjective usage.
🔎 Proposed fix
-Third party projects that were included in Tactility retain their licenses. +Third-party projects that were included in Tactility retain their licenses.TactilityCore/Include/Tactility/TactilityCore.h (1)
5-10: Inconsistent include styles for internal headers.Line 5 uses quoted includes (
"Tactility/Thread.h") while line 10 uses angle brackets (<Tactility/kernel/Kernel.h>). Consider using a consistent style for internal project headers.TactilityFreeRtos/Include/Tactility/Semaphore.h (1)
61-61: Non-standard move constructor signature.This constructor takes a non-const lvalue reference and moves from it, which is the behavior of a move constructor but with a non-standard signature. This can cause confusion and won't work with rvalue temporaries.
Consider using the standard move constructor signature.
🔎 Proposed fix
- Semaphore(Semaphore& other) : handle(std::move(other.handle)) {} + Semaphore(Semaphore&& other) noexcept : handle(std::move(other.handle)) {}TactilityFreeRtos/Include/Tactility/Thread.h (1)
164-168: Scoped lock acquired but not utilized correctly.
asScopedLock()returns an unlockedScopedLock. The manuallock.lock()call works, but returningstateafter the function exits means the lock is still held during the return. Consider simplifying with a directmutex.lock()/unlock()pattern or ensuring the scoped lock is used idiomatically.🔎 Proposed simplification
State getState() const { - auto lock = mutex.asScopedLock(); - lock.lock(); - return state; + mutex.lock(); + State result = state; + mutex.unlock(); + return result; }Tests/TactilityFreeRtos/ThreadTest.cpp (1)
44-63: Update the test case name to reflect the API change.The test case name still references "thread id" but the implementation now tests
getTaskHandle(). Consider renaming to accurately describe what is being tested.🔎 Proposed fix
-TEST_CASE("thread id should only be set at when thread is started") { +TEST_CASE("thread task handle should only be set when thread is started") {TactilityCore/Include/Tactility/kernel/Platform.h (1)
6-9: Consider usingenum classfor better type safety.The current
typedef enumis valid butenum classprovides stronger type safety and scoped enumerators in modern C++.🔎 Proposed refactor
-/** Recognized platform types */ -typedef enum { - PlatformEsp, - PlatformSimulator -} Platform; +/** Recognized platform types */ +enum class Platform { + Esp, + Simulator +};Note: This would require updating call sites from
PlatformEsptoPlatform::EspandPlatformSimulatortoPlatform::Simulator.Tests/TactilityFreeRtos/RecursiveMutexTest.cpp (1)
22-28: Consider: Timing-based assertions may be flaky under load.The 5ms delays assume the scheduler will have run by then. In heavily loaded CI environments, this could occasionally fail. Consider using longer delays or event-based synchronization for more deterministic behavior.
TactilityFreeRtos/Include/Tactility/Mutex.h (1)
39-43: Changedunlock()to void, ignoringxSemaphoreGivereturn value.The change aligns with the interface update. Note that
xSemaphoreGivecan returnpdFAILif called on a mutex not owned by the current task. Since the mutex enforces non-ISR context via the assertion and is designed for proper lock/unlock pairing, this should be safe in correct usage.If defensive error detection is desired, consider adding an assertion on the return value:
[[maybe_unused]] auto result = xSemaphoreGive(handle.get()); assert(result == pdPASS);Tests/TactilityFreeRtos/Main.cpp (1)
26-30: Consider returning early aftervTaskEndScheduler()to avoid redundantvTaskDelete.When
shouldExit()is true (e.g.,--helpor--exitflags), the scheduler ends but the task still callsvTaskDelete(nullptr). Depending on the FreeRTOS port, this may be harmless or cause undefined behavior after the scheduler has stopped.🔎 Proposed fix
if (context.shouldExit()) { // important - query flags (and --exit) rely on the user doing this vTaskEndScheduler(); + return; } vTaskDelete(nullptr);TactilityFreeRtos/Include/Tactility/Dispatcher.h (1)
28-29: Type mismatch:BACKPRESSURE_WARNING_COUNTshould besize_toruint32_t.
EventBits_tis semantically incorrect here since this value is compared againstqueue.size()(which returnssize_t), not used as event bits.🔎 Proposed fix
- static constexpr EventBits_t BACKPRESSURE_WARNING_COUNT = 100U; + static constexpr size_t BACKPRESSURE_WARNING_COUNT = 100U;Tests/TactilityFreeRtos/CMakeLists.txt (3)
5-5: Hardcodingg++reduces portability.This forces
g++regardless of the toolchain configured by the parent project or CI environment. Consider removing this line to inherit the compiler from the parent CMake configuration.🔎 Proposed fix
-set(CMAKE_CXX_COMPILER g++)
10-11: Prefertarget_compile_definitionsoveradd_definitions.
add_definitionsaffects all targets in the directory scope. Usingtarget_compile_definitionsis the modern CMake approach and limits the scope to the specific target.🔎 Proposed fix
-add_definitions(-D_Nullable=) -add_definitions(-D_Nonnull=) +target_compile_definitions(TactilityFreeRtosTests PRIVATE + _Nullable= + _Nonnull= +)Note: Move this after the
add_executablecall.
21-24: UsePRIVATElinkage for test executable.Test executables typically don't export their linked libraries.
PRIVATEis more appropriate here.🔎 Proposed fix
-target_link_libraries(TactilityFreeRtosTests PUBLIC +target_link_libraries(TactilityFreeRtosTests PRIVATE TactilityFreeRtos freertos_kernel )TactilityFreeRtos/Include/Tactility/Lock.h (1)
46-46: Inconsistent indentation: tabs mixed with spaces.Lines 46, 79-80 use tabs while the rest of the file uses spaces.
Also applies to: 79-81
Tactility/Source/hal/gps/GpsInit.cpp (2)
46-74: Buffer size validation missing in makeCASPacket.The function writes to
bufferwithout validating that the caller-provided buffer is large enough. The function writes to offset9 + payload_size, requiring a buffer of at leastpayload_size + 10bytes. Consider adding abuffer_sizeparameter and validation.🔎 Suggested defensive improvement
-static uint8_t makeCASPacket(uint8_t* buffer, uint8_t class_id, uint8_t msg_id, uint8_t payload_size, const uint8_t *msg) +static uint8_t makeCASPacket(uint8_t* buffer, size_t buffer_size, uint8_t class_id, uint8_t msg_id, uint8_t payload_size, const uint8_t *msg) { + size_t required_size = static_cast<size_t>(payload_size) + 10; + assert(buffer_size >= required_size); + // ... rest of function
139-168: Unreachable code after exhaustive switch.The
TT_LOG_Iandreturn falseat lines 166-167 are unreachable since the switch covers allGpsModelenum values. IfGpsModel::Unknowncauses a crash viatt_crash(), and all other cases return, this code path cannot be reached. Consider removing this dead code or adding adefaultcase if future enum values are expected.TactilityC/Include/tt_timer.h (2)
48-55: Inconsistent indentation in doc comment.The doc comment for
tt_timer_resethas extra indentation that doesn't match the rest of the file's formatting style.🔎 Proposed fix
bool tt_timer_reset_with_interval(TimerHandle handle, TickType interval); - /** +/** * Restart an already started timer * @param[in] handle the timer instance handle * @return true when the timer was successfully restarted */ bool tt_timer_reset(TimerHandle handle);
86-86: Inconsistent type usage:TickType_tvsTickType.This function uses
TickType_twhile other functions in this header useTickType. For API consistency, consider usingTickTypethroughout.🔎 Proposed fix
-bool tt_timer_set_pending_callback(TimerHandle handle, TimerPendingCallback callback, void* callbackContext, uint32_t callbackArg, TickType_t timeout); +bool tt_timer_set_pending_callback(TimerHandle handle, TimerPendingCallback callback, void* callbackContext, uint32_t callbackArg, TickType timeout);TactilityFreeRtos/Include/Tactility/DispatcherThread.h (1)
64-65:isStarted()logic may not accurately reflect running state.The condition
thread != nullptr && !interruptThreaddoesn't account for the thread being in the process of stopping (afterstop()is called but beforejoin()completes). Consider checkingthread->getState()for a more accurate status.TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (2)
27-33:constexpris incorrect for runtime function calls.Functions like
getTicks()callxTaskGetTickCount()which are runtime FreeRTOS calls. These cannot beconstexprin the traditional sense. While some compilers may allow this as an extension, it's misleading and non-portable.🔎 Proposed fix
/** @return the amount of ticks that has passed in the main kernel task */ -constexpr TickType_t getTicks() { +inline TickType_t getTicks() { if (xPortInIsrContext() == pdTRUE) { return xTaskGetTickCountFromISR(); } else { return xTaskGetTickCount(); } }Similar changes should be applied to
getMillis(),getMicrosSinceBoot(),delayTicks(),delayMillis(), anddelayMicros().
52-55: Incorrect seconds-to-ticks conversion formula.The formula
seconds * 1000U / portTICK_PERIOD_MSis correct only ifportTICK_PERIOD_MS == 1. The standard formula should beseconds * configTICK_RATE_HZorseconds * 1000 / portTICK_PERIOD_MS(which is what you have, but the intermediate calculation may overflow for large values).Consider using
configTICK_RATE_HZdirectly for clarity:🔎 Proposed fix for clarity and overflow safety
/** Convert seconds to ticks */ constexpr TickType_t secondsToTicks(uint32_t seconds) { - return static_cast<TickType_t>(seconds) * 1000U / portTICK_PERIOD_MS; + return static_cast<TickType_t>(seconds) * configTICK_RATE_HZ; }TactilityFreeRtos/Include/Tactility/Timer.h (2)
129-136: ISR path ignorespxHigherPriorityTaskWokenresult.When calling
xTimerPendFunctionCallFromISR, thepxHigherPriorityTaskWokenparameter is passed asnullptr, meaning no context switch is triggered even if a higher-priority task becomes ready. This could delay callback execution.🔎 Proposed fix
bool setPendingCallback(PendingCallback newCallback, void* callbackContext, uint32_t callbackArg, TickType_t timeout) const { if (xPortInIsrContext() == pdTRUE) { assert(timeout == 0); - return xTimerPendFunctionCallFromISR(newCallback, callbackContext, callbackArg, nullptr) == pdPASS; + BaseType_t xHigherPriorityTaskWoken = pdFALSE; + BaseType_t result = xTimerPendFunctionCallFromISR(newCallback, callbackContext, callbackArg, &xHigherPriorityTaskWoken); + portYIELD_FROM_ISR(xHigherPriorityTaskWoken); + return result == pdPASS; } else { return xTimerPendFunctionCall(newCallback, callbackContext, callbackArg, timeout) == pdPASS; } }
138-148:setCallbackPriorityaffects ALL timers globally.This method modifies the priority of the timer daemon task, which affects the execution priority of all timer callbacks in the system, not just this timer instance. The documentation should clarify this global side effect.
🔎 Suggested documentation improvement
- /** Set callback priority + /** Set callback priority for the timer daemon task. + * @warning This affects ALL timer callbacks in the system, not just this timer. * @param[in] priority The priority */ void setCallbackPriority(Thread::Priority priority) const {TactilityFreeRtos/Include/Tactility/EventGroup.h (1)
49-73: Unused variable and ISR semantics clarification.Line 52 declares
uint32_t resultbut never uses it. Also, in the ISR path,outFlagsis set to the inputflagsrather than actual resulting flags (line 61), which is unavoidable sincexEventGroupSetBitsFromISRdefers to the timer daemon. Consider clarifying this in the documentation or removingoutFlagsfor ISR context.🔎 Proposed fix
bool set(uint32_t flags, uint32_t* outFlags = nullptr, Error* outError = nullptr) const { assert(handle); if (xPortInIsrContext() == pdTRUE) { - uint32_t result; BaseType_t yield = pdFALSE; if (xEventGroupSetBitsFromISR(handle.get(), flags, &yield) == pdFAIL) { if (outError != nullptr) { *outError = Error::Resource; } return false; } else { - if (outFlags != nullptr) { - *outFlags = flags; - } + // Note: outFlags not set in ISR context as bits are set deferred by timer daemon portYIELD_FROM_ISR(yield); return true; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (148)
.github/workflows/tests.ymlCMakeLists.txtDevices/simulator/Source/LvglTask.cppDevices/simulator/Source/Main.cppFirmware/CMakeLists.txtLICENSE.mdTactility/CMakeLists.txtTactility/Include/Tactility/Tactility.hTactility/Include/Tactility/hal/gps/GpsDevice.hTactility/Include/Tactility/hal/gps/Satellites.hTactility/Include/Tactility/hal/i2c/I2c.hTactility/Include/Tactility/hal/sdcard/SdCardDevice.hTactility/Include/Tactility/hal/spi/Spi.hTactility/Include/Tactility/hal/uart/Uart.hTactility/Include/Tactility/service/gps/GpsService.hTactility/Include/Tactility/service/loader/Loader.hTactility/Include/Tactility/service/wifi/Wifi.hTactility/Private/Tactility/app/AppInstance.hTactility/Private/Tactility/app/i2cscanner/I2cScannerPrivate.hTactility/Private/Tactility/service/gui/GuiService.hTactility/Private/Tactility/service/memorychecker/MemoryCheckerService.hTactility/Private/Tactility/service/screenshot/ScreenshotTask.hTactility/Source/Tactility.cppTactility/Source/app/AppManifestParsing.cppTactility/Source/app/development/Development.cppTactility/Source/app/files/State.cppTactility/Source/app/files/View.cppTactility/Source/app/fileselection/State.cppTactility/Source/app/fileselection/View.cppTactility/Source/app/gpssettings/GpsSettings.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactility/Source/app/power/Power.cppTactility/Source/app/screenshot/Screenshot.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/hal/Device.cppTactility/Source/hal/gps/GpsDevice.cppTactility/Source/hal/gps/GpsInit.cppTactility/Source/hal/gps/Satellites.cppTactility/Source/hal/gps/Ublox.cppTactility/Source/hal/i2c/I2c.cppTactility/Source/hal/spi/Spi.cppTactility/Source/hal/uart/UartEsp.cppTactility/Source/hal/uart/UartPosix.cppTactility/Source/kernel/SystemEvents.cppTactility/Source/lvgl/EspLvglPort.cppTactility/Source/lvgl/LvglSync.cppTactility/Source/lvgl/Statusbar.cppTactility/Source/network/Http.cppTactility/Source/network/HttpServer.cppTactility/Source/service/ServiceRegistration.cppTactility/Source/service/displayidle/DisplayIdle.cppTactility/Source/service/espnow/EspNowWifi.cppTactility/Source/service/gui/GuiService.cppTactility/Source/service/keyboardidle/KeyboardIdle.cppTactility/Source/service/memorychecker/MemoryCheckerService.cppTactility/Source/service/screenshot/Screenshot.cppTactility/Source/service/screenshot/ScreenshotTask.cppTactility/Source/service/sdcard/Sdcard.cppTactility/Source/service/statusbar/Statusbar.cppTactility/Source/service/wifi/Wifi.cppTactility/Source/service/wifi/WifiEsp.cppTactility/Source/service/wifi/WifiMock.cppTactilityC/Include/tt_hal_i2c.hTactilityC/Include/tt_kernel.hTactilityC/Include/tt_lock.hTactilityC/Include/tt_message_queue.hTactilityC/Include/tt_thread.hTactilityC/Include/tt_timer.hTactilityC/Source/symbols/freertos.cppTactilityC/Source/tt_app.cppTactilityC/Source/tt_hal_i2c.cppTactilityC/Source/tt_init.cppTactilityC/Source/tt_kernel.cppTactilityC/Source/tt_lock.cppTactilityC/Source/tt_message_queue.cppTactilityC/Source/tt_thread.cppTactilityC/Source/tt_timer.cppTactilityCore/CMakeLists.txtTactilityCore/Include/Tactility/CpuAffinity.hTactilityCore/Include/Tactility/Dispatcher.hTactilityCore/Include/Tactility/DispatcherThread.hTactilityCore/Include/Tactility/EventFlag.hTactilityCore/Include/Tactility/MessageQueue.hTactilityCore/Include/Tactility/RtosCompat.hTactilityCore/Include/Tactility/RtosCompatEventGroups.hTactilityCore/Include/Tactility/RtosCompatSemaphore.hTactilityCore/Include/Tactility/RtosCompatTask.hTactilityCore/Include/Tactility/RtosCompatTimers.hTactilityCore/Include/Tactility/Semaphore.hTactilityCore/Include/Tactility/StreamBuffer.hTactilityCore/Include/Tactility/TactilityCore.hTactilityCore/Include/Tactility/TactilityCoreConfig.hTactilityCore/Include/Tactility/Thread.hTactilityCore/Include/Tactility/Timer.hTactilityCore/Include/Tactility/file/File.hTactilityCore/Include/Tactility/kernel/Kernel.hTactilityCore/Include/Tactility/kernel/Platform.hTactilityCore/Source/Check.cppTactilityCore/Source/Dispatcher.cppTactilityCore/Source/DispatcherThread.cppTactilityCore/Source/EventFlag.cppTactilityCore/Source/Lock.cppTactilityCore/Source/MessageQueue.cppTactilityCore/Source/Semaphore.cppTactilityCore/Source/StreamBuffer.cppTactilityCore/Source/Thread.cppTactilityCore/Source/Timer.cppTactilityCore/Source/file/File.cppTactilityCore/Source/kernel/Kernel.cppTactilityCore/Source/kernel/Platform.cppTactilityCore/Source/kernel/critical/Critical.cppTactilityFreeRtos/CMakeLists.txtTactilityFreeRtos/Include/Tactility/Dispatcher.hTactilityFreeRtos/Include/Tactility/DispatcherThread.hTactilityFreeRtos/Include/Tactility/EventGroup.hTactilityFreeRtos/Include/Tactility/Lock.hTactilityFreeRtos/Include/Tactility/MessageQueue.hTactilityFreeRtos/Include/Tactility/Mutex.hTactilityFreeRtos/Include/Tactility/PubSub.hTactilityFreeRtos/Include/Tactility/RecursiveMutex.hTactilityFreeRtos/Include/Tactility/Semaphore.hTactilityFreeRtos/Include/Tactility/Thread.hTactilityFreeRtos/Include/Tactility/Timer.hTactilityFreeRtos/Include/Tactility/freertoscompat/EventGroups.hTactilityFreeRtos/Include/Tactility/freertoscompat/PortCompat.hTactilityFreeRtos/Include/Tactility/freertoscompat/Queue.hTactilityFreeRtos/Include/Tactility/freertoscompat/README.mdTactilityFreeRtos/Include/Tactility/freertoscompat/RTOS.hTactilityFreeRtos/Include/Tactility/freertoscompat/Semaphore.hTactilityFreeRtos/Include/Tactility/freertoscompat/Task.hTactilityFreeRtos/Include/Tactility/freertoscompat/Timers.hTactilityFreeRtos/Include/Tactility/kernel/Kernel.hTactilityFreeRtos/README.mdTests/CMakeLists.txtTests/TactilityCore/CMakeLists.txtTests/TactilityFreeRtos/CMakeLists.txtTests/TactilityFreeRtos/DispatcherTest.cppTests/TactilityFreeRtos/DispatcherThreadTest.cppTests/TactilityFreeRtos/LockTest.cppTests/TactilityFreeRtos/Main.cppTests/TactilityFreeRtos/MessageQueueTest.cppTests/TactilityFreeRtos/MutexTest.cppTests/TactilityFreeRtos/PubSubTest.cppTests/TactilityFreeRtos/RecursiveMutexTest.cppTests/TactilityFreeRtos/SemaphoreTest.cppTests/TactilityFreeRtos/ThreadTest.cppTests/TactilityFreeRtos/TimerTest.cpp
💤 Files with no reviewable changes (30)
- TactilityCore/Include/Tactility/TactilityCoreConfig.h
- TactilityCore/Source/Lock.cpp
- Tests/TactilityFreeRtos/DispatcherThreadTest.cpp
- TactilityCore/Include/Tactility/Dispatcher.h
- Tests/TactilityFreeRtos/MessageQueueTest.cpp
- TactilityCore/Source/Dispatcher.cpp
- TactilityCore/Include/Tactility/Thread.h
- TactilityCore/Include/Tactility/Semaphore.h
- TactilityCore/Source/kernel/Kernel.cpp
- TactilityCore/Include/Tactility/RtosCompatEventGroups.h
- TactilityCore/Include/Tactility/kernel/Kernel.h
- TactilityC/Include/tt_kernel.h
- TactilityCore/Include/Tactility/StreamBuffer.h
- TactilityCore/Include/Tactility/DispatcherThread.h
- TactilityCore/Source/StreamBuffer.cpp
- TactilityCore/Source/DispatcherThread.cpp
- TactilityCore/Include/Tactility/MessageQueue.h
- TactilityCore/Include/Tactility/RtosCompatTask.h
- TactilityCore/Source/MessageQueue.cpp
- TactilityCore/Source/Semaphore.cpp
- TactilityCore/Include/Tactility/Timer.h
- TactilityCore/Include/Tactility/RtosCompatSemaphore.h
- TactilityCore/Include/Tactility/RtosCompatTimers.h
- TactilityCore/Include/Tactility/RtosCompat.h
- TactilityCore/Include/Tactility/EventFlag.h
- TactilityC/Source/tt_kernel.cpp
- Tests/TactilityFreeRtos/TimerTest.cpp
- TactilityCore/Source/Timer.cpp
- TactilityCore/Source/Thread.cpp
- TactilityCore/Source/EventFlag.cpp
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/hal/gps/GpsDevice.cppTactilityCore/CMakeLists.txtTactilityFreeRtos/Include/Tactility/freertoscompat/Task.hTactility/Include/Tactility/service/wifi/Wifi.hTactility/Include/Tactility/hal/i2c/I2c.hTactility/CMakeLists.txtTactility/Source/service/wifi/Wifi.cppTactilityCore/Source/kernel/critical/Critical.cppTactilityFreeRtos/Include/Tactility/freertoscompat/Queue.hTactilityCore/Include/Tactility/CpuAffinity.hTactilityFreeRtos/CMakeLists.txtTactility/Source/hal/spi/Spi.cppTactilityFreeRtos/Include/Tactility/freertoscompat/README.mdTactility/Source/hal/gps/Ublox.cppTactilityCore/Include/Tactility/file/File.hTactilityC/Source/symbols/freertos.cppTactility/Source/lvgl/EspLvglPort.cppTactilityCore/Source/Check.cppTactilityCore/Include/Tactility/TactilityCore.hTactility/Include/Tactility/hal/gps/Satellites.hTactility/Source/service/ServiceRegistration.cppTactility/Include/Tactility/hal/spi/Spi.hTactility/Source/service/espnow/EspNowWifi.cppTactility/Source/Tactility.cppTactilityFreeRtos/Include/Tactility/freertoscompat/RTOS.hTactility/Source/app/development/Development.cppCMakeLists.txtTactilityC/Source/tt_init.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/hal/gps/GpsDevice.cppTactility/Include/Tactility/hal/i2c/I2c.hTactility/Source/hal/Device.cppTactility/Source/hal/gps/Ublox.cppTactilityCore/Source/Check.cppDevices/simulator/Source/LvglTask.cppTactility/Source/hal/uart/UartEsp.cppTactility/Source/hal/gps/GpsInit.cppTactility/Source/hal/i2c/I2c.cppTactility/Private/Tactility/app/i2cscanner/I2cScannerPrivate.hTactility/Source/app/development/Development.cppTactility/Source/app/gpssettings/GpsSettings.cppTactility/Source/hal/uart/UartPosix.cpp
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
TactilityC/Include/tt_hal_i2c.hTactility/Include/Tactility/hal/i2c/I2c.hTactilityC/Source/tt_hal_i2c.cppTactility/Source/app/files/View.cppTactility/Source/service/keyboardidle/KeyboardIdle.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/hal/spi/Spi.cppTactility/Source/service/displayidle/DisplayIdle.cppTactility/Source/lvgl/EspLvglPort.cppTactility/Include/Tactility/hal/sdcard/SdCardDevice.hDevices/simulator/Source/LvglTask.cppTactility/Source/app/power/Power.cppTactility/Include/Tactility/hal/spi/Spi.hTactility/Source/hal/i2c/I2c.cppTactility/Source/lvgl/Statusbar.cppTactility/Source/app/development/Development.cppCMakeLists.txtTactilityC/Source/tt_init.cppTactility/Source/lvgl/LvglSync.cpp
📚 Learning: 2025-11-11T17:22:20.238Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 411
File: sdkconfig.board.waveshare-s3-touch-lcd-43:39-39
Timestamp: 2025-11-11T17:22:20.238Z
Learning: In Tactility sdkconfig.board.* files, board names (CONFIG_TT_BOARD_NAME) that include screen size measurements intentionally use escaped quotes (e.g., "WaveShare S3 Touch LCD 4.3\"") where the \" represents the inch symbol (") in the final board name string. This is not a syntax error but deliberate formatting.
Applied to files:
Tactility/Private/Tactility/service/screenshot/ScreenshotTask.h
📚 Learning: 2025-11-11T17:22:17.736Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 411
File: sdkconfig.board.waveshare-s3-lcd-13:39-39
Timestamp: 2025-11-11T17:22:17.736Z
Learning: In sdkconfig.board.* files for Tactility boards, CONFIG_TT_BOARD_NAME values that include display sizes in inches intentionally use a trailing escaped quote (e.g., "WaveShare S3 LCD 1.3\"") to display the literal inch symbol (") in the board name. This pattern is correct and should not be flagged as a string escaping error.
Applied to files:
Tactility/Private/Tactility/service/screenshot/ScreenshotTask.h
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/timezone/TimeZone.cppTactility/Source/lvgl/EspLvglPort.cppDevices/simulator/Source/LvglTask.cppTactility/Source/app/power/Power.cppTactility/Source/lvgl/Statusbar.cpp
📚 Learning: 2025-10-28T19:44:35.263Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Drivers/ST7789-i8080/Source/St7789i8080Display.cpp:326-329
Timestamp: 2025-10-28T19:44:35.263Z
Learning: In the LilyGo T-Display S3 ST7789 i8080 display driver (Drivers/ST7789-i8080/Source/St7789i8080Display.cpp), both callback registrations are required: setting on_color_trans_done during panel IO creation in configurePanelIO() and calling esp_lcd_panel_io_register_event_callbacks() in initializeLvgl() after the LVGL display is created. This dual registration is intentional and necessary for proper operation on this hardware.
Applied to files:
Tactility/Source/service/displayidle/DisplayIdle.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Tactility/Source/lvgl/EspLvglPort.cpp
🧬 Code graph analysis (23)
TactilityC/Include/tt_lock.h (1)
TactilityC/Source/tt_lock.cpp (2)
tt_lock_release(35-37)tt_lock_release(35-35)
Tactility/Private/Tactility/service/gui/GuiService.h (3)
Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)Tactility/Source/service/sdcard/Sdcard.cpp (1)
mutex(25-27)Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)
Tactility/Source/service/keyboardidle/KeyboardIdle.cpp (1)
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
millisToTicks(58-64)
TactilityC/Include/tt_thread.h (1)
TactilityC/Source/tt_thread.cpp (4)
tt_thread_join(65-67)tt_thread_join(65-65)tt_thread_get_task_handle(69-71)tt_thread_get_task_handle(69-69)
Tactility/Source/service/sdcard/Sdcard.cpp (2)
Tactility/Source/service/statusbar/Statusbar.cpp (2)
service(280-283)service(280-280)Tactility/Source/service/wifi/WifiEsp.cpp (4)
service(917-936)service(917-917)service(938-962)service(938-938)
Tests/TactilityFreeRtos/MutexTest.cpp (2)
TactilityFreeRtos/Include/Tactility/Mutex.h (1)
Mutex(24-26)TactilityFreeRtos/Include/Tactility/Thread.h (1)
Thread(18-162)
Tactility/Source/kernel/SystemEvents.cpp (1)
Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)
TactilityFreeRtos/Include/Tactility/MessageQueue.h (2)
TactilityFreeRtos/Include/Tactility/Timer.h (1)
reset(104-113)TactilityCore/Include/Tactility/MessageQueue.h (2)
MessageQueue(26-84)tt(20-86)
Tactility/Source/service/displayidle/DisplayIdle.cpp (1)
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
millisToTicks(58-64)
Tactility/Source/service/screenshot/ScreenshotTask.cpp (3)
Tactility/Source/app/timezone/TimeZone.cpp (1)
mutex(79-89)Tactility/Source/service/sdcard/Sdcard.cpp (1)
mutex(25-27)Tactility/Source/service/statusbar/Statusbar.cpp (2)
mutex(153-155)mutex(157-159)
TactilityC/Include/tt_timer.h (1)
TactilityC/Source/tt_timer.cpp (8)
tt_timer_alloc(12-16)tt_timer_alloc(12-12)tt_timer_reset_with_interval(32-34)tt_timer_reset_with_interval(32-32)tt_timer_reset(28-30)tt_timer_reset(28-28)tt_timer_get_expiry_time(44-46)tt_timer_get_expiry_time(44-44)
Tactility/Include/Tactility/hal/sdcard/SdCardDevice.h (2)
Tactility/Source/hal/sdcard/SdmmcDevice.cpp (2)
getState(98-119)getState(98-98)Tactility/Source/hal/sdcard/SpiSdCardDevice.cpp (2)
getState(126-147)getState(126-126)
TactilityFreeRtos/Include/Tactility/Semaphore.h (1)
TactilityFreeRtos/Include/Tactility/Lock.h (1)
Lock(14-47)
Tactility/Source/app/power/Power.cpp (2)
TactilityFreeRtos/Include/Tactility/Timer.h (1)
Timer(15-65)TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
millisToTicks(58-64)
TactilityFreeRtos/Include/Tactility/Mutex.h (1)
TactilityFreeRtos/Include/Tactility/MessageQueue.h (1)
get(88-107)
TactilityFreeRtos/Include/Tactility/RecursiveMutex.h (1)
TactilityFreeRtos/Include/Tactility/MessageQueue.h (1)
get(88-107)
Tactility/Source/app/systeminfo/SystemInfo.cpp (2)
TactilityFreeRtos/Include/Tactility/Timer.h (1)
Timer(15-65)TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
millisToTicks(58-64)
TactilityFreeRtos/Include/Tactility/Thread.h (1)
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (2)
getTicks(27-33)delayTicks(70-77)
TactilityFreeRtos/Include/Tactility/Timer.h (2)
TactilityFreeRtos/Include/Tactility/MessageQueue.h (1)
get(88-107)TactilityFreeRtos/Include/Tactility/Thread.h (1)
Priority(31-118)
Tactility/Source/app/development/Development.cpp (2)
TactilityFreeRtos/Include/Tactility/Timer.h (1)
Timer(15-65)Tactility/Source/app/gpssettings/GpsSettings.cpp (2)
timer(64-67)timer(69-72)
Tactility/Source/app/gpssettings/GpsSettings.cpp (1)
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
secondsToTicks(53-55)
TactilityFreeRtos/Include/Tactility/EventGroup.h (2)
Tactility/Include/Tactility/app/App.h (1)
result(25-75)TactilityFreeRtos/Include/Tactility/MessageQueue.h (1)
get(88-107)
Tactility/Source/lvgl/LvglSync.cpp (1)
TactilityCore/Source/file/File.cpp (2)
timeout(17-17)timeout(17-17)
🪛 Clang (14.0.6)
TactilityCore/Include/Tactility/kernel/Platform.h
[error] 3-3: unknown type name 'namespace'
(clang-diagnostic-error)
[error] 3-3: expected ';' after top level declarator
(clang-diagnostic-error)
[error] 3-3: expected identifier or '('
(clang-diagnostic-error)
🪛 LanguageTool
LICENSE.md
[grammar] ~10-~10: Use a hyphen to join words.
Context: ...me](https://kenvanhoeylandt.net). Third party projects that were included in Tac...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: Linux
- GitHub Check: run
- GitHub Check: macOS
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/TactilityFreeRtos/TimerTest.cpp (1)
6-18: Consider a more precise assertion to better validate periodic behavior.The test checks
CHECK_GE(counter, 2)after two 10-tick runs with a 1-tick period timer. With approximately 20 expected callbacks, checking for only >= 2 is very conservative and doesn't validate that the timer is actually firing periodically as expected. Consider tightening this to something likeCHECK_GE(counter, 16)to catch timing issues while still allowing for FreeRTOS scheduling variance.🔎 Suggested improvement
- CHECK_GE(counter, 2); + CHECK_GE(counter, 16); // Expect ~20 callbacks (2 runs × ~10 each), allow margin for timing
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
TactilityFreeRtos/Include/Tactility/Semaphore.hTests/TactilityFreeRtos/TimerTest.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- TactilityFreeRtos/Include/Tactility/Semaphore.h
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/TactilityFreeRtos/TimerTest.cpp (1)
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
delayTicks(70-77)
🔇 Additional comments (3)
Tests/TactilityFreeRtos/TimerTest.cpp (3)
20-30: LGTM! Precise validation of periodic timer behavior.The test correctly validates that a periodic timer with a 1-tick interval fires exactly 10 times over 10 ticks. The precise assertion properly tests the timer's periodic behavior.
32-44: LGTM! Correctly validates once-timer restart behavior.The test properly verifies that a
Timer::Type::Oncetimer can be restarted and fires exactly once per start cycle. The assertion correctly expects 2 total callbacks.
8-9: LGTM! Tests correctly use the new Timer API.The tests properly reflect the new Timer API changes:
- Constructor specifies interval at construction time:
Timer(Type, interval, callback)start()is called without parameters and returns a booleanstop()returns a booleanAll tests correctly verify the return values from
start()andstop().Also applies to: 11-12, 14-14, 23-24, 26-26, 34-35, 37-38, 40-40
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
TactilityFreeRtos/Include/Tactility/Dispatcher.h (2)
46-51: Destructor race condition remains unresolved.The destructor's synchronization is insufficient. A consumer thread could be executing a function (line 115) with the mutex released, allowing the destructor to acquire the lock, complete destruction, and invalidate object state while the consumer still references member variables or the function object itself.
93-129: Potential busy-wait loop remains unresolved.The hardcoded 10-tick timeout on line 107 can lead to a busy-wait if the mutex is held for extended periods. While the
shutdownflag (line 126) will eventually terminate the loop, the consumer may spin many times, consuming CPU unnecessarily.TactilityFreeRtos/Include/Tactility/Thread.h (1)
171-182: Race condition: state transition not protected by mutex.The mutex is released at line 176 before
setState(State::Starting)at line 178. This leaves a window where state can be observed or modified inconsistently. The state transition and task creation should be atomic.
🧹 Nitpick comments (5)
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
57-64: Prefer integer arithmetic inmillisToTicks.Line 62 uses floating-point arithmetic for tick conversion when
configTICK_RATE_HZ != 1000. Integer arithmetic is generally preferred in embedded systems for better performance and determinism.🔎 Proposed integer-based implementation
/** Convert milliseconds to ticks */ constexpr TickType_t millisToTicks(uint32_t milliSeconds) { #if configTICK_RATE_HZ == 1000 return static_cast<TickType_t>(milliSeconds); #else - return static_cast<TickType_t>(((float)configTICK_RATE_HZ) / 1000.0f * (float)milliSeconds); + return static_cast<TickType_t>((static_cast<uint64_t>(milliSeconds) * configTICK_RATE_HZ) / 1000U); #endif }Note: Using
uint64_tintermediate to avoid overflow for largemilliSecondsvalues.TactilityFreeRtos/Include/Tactility/EventGroup.h (2)
49-73: Remove unused variable.Line 52 declares
uint32_t resultbut never uses it in the ISR path. This should be removed.🔎 Proposed fix
bool set(uint32_t flags, uint32_t* outFlags = nullptr, Error* outError = nullptr) const { assert(handle); if (xPortInIsrContext() == pdTRUE) { - uint32_t result; BaseType_t yield = pdFALSE; if (xEventGroupSetBitsFromISR(handle.get(), flags, &yield) == pdFAIL) {
82-83: Add handle assertion for consistency.The
clear()method should includeassert(handle)at the beginning, similar to theset()method (line 50), to maintain consistent defensive checks across the API.🔎 Proposed fix
bool clear(uint32_t flags, uint32_t* outFlags = nullptr, Error* outError = nullptr) const { + assert(handle); if (xPortInIsrContext() == pdTRUE) {TactilityFreeRtos/Include/Tactility/Thread.h (2)
90-97: Consider invoking callback outside the mutex to avoid potential deadlock.The
stateCallbackis invoked while the mutex is held. If the callback attempts to call any Thread method that acquires the same mutex, it will deadlock.🔎 Proposed fix
void setState(State newState) { + StateCallback cb = nullptr; + void* ctx = nullptr; mutex.lock(); state = newState; - if (stateCallback) { - stateCallback(state, stateCallbackContext); - } + cb = stateCallback; + ctx = stateCallbackContext; mutex.unlock(); + if (cb) { + cb(newState, ctx); + } }
239-239: Consider adding synchronization forgetTaskHandle().
taskHandleis modified inmainBody(line 73) without holding the mutex, whilegetTaskHandle()reads it without synchronization. While pointer reads are typically atomic on most platforms, this is technically a data race. Thejoin()method relies on this for its polling loop.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Drivers/BQ25896/Source/Bq25896.cppDrivers/DRV2605/Source/Drv2605.hTactilityC/Source/tt_lock.cppTactilityCore/Source/kernel/critical/Critical.cppTactilityFreeRtos/Include/Tactility/Dispatcher.hTactilityFreeRtos/Include/Tactility/EventGroup.hTactilityFreeRtos/Include/Tactility/Thread.hTactilityFreeRtos/Include/Tactility/kernel/Kernel.hTests/TactilityCore/Main.cppTests/TactilityFreeRtos/Main.cpp
✅ Files skipped from review due to trivial changes (1)
- Drivers/DRV2605/Source/Drv2605.h
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/TactilityFreeRtos/Main.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Drivers/BQ25896/Source/Bq25896.cpp
🧬 Code graph analysis (2)
TactilityFreeRtos/Include/Tactility/EventGroup.h (3)
TactilityFreeRtos/Include/Tactility/Dispatcher.h (1)
tt(18-85)TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
tt(17-99)TactilityCore/Source/EventFlag.cpp (1)
EventFlag(9-135)
TactilityFreeRtos/Include/Tactility/Thread.h (1)
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (3)
tt(17-99)getTicks(27-33)delayTicks(70-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: run
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
🔇 Additional comments (18)
TactilityC/Source/tt_lock.cpp (1)
35-37: LGTM! Previous issue resolved.The critical issue identified in the previous review has been fixed. The function now correctly calls
unlock()without thereturnkeyword, matching thevoidreturn type.TactilityCore/Source/kernel/critical/Critical.cpp (4)
1-4: LGTM! Header includes updated correctly.The include paths have been properly updated to use the new
freertoscompat/Task.hheader from the TactilityFreeRtos subproject, which provides the necessary FreeRTOS API declarations.
6-13: LGTM! Critical bug fix:TT_EXIT_CRITICAL()macro now defined.The addition of the
TT_EXIT_CRITICAL()macro for both ESP and non-ESP platforms addresses the critical bug identified in the previous review. The macros are correctly defined and symmetric with theirTT_ENTER_CRITICAL()counterparts.
17-33: LGTM! ISR detection updated to use FreeRTOS API directly.The change from
kernel::isIsr()toxPortInIsrContext() == pdTRUEat line 20 is consistent with the PR's goal of using FreeRTOS compatibility APIs directly. The logic remains correct and well-structured.
35-43: LGTM! Critical bug fixed: Line 39 now correctly callsTT_EXIT_CRITICAL().The critical bug identified in the previous review has been resolved. Line 39 now correctly calls
TT_EXIT_CRITICAL()instead ofTT_ENTER_CRITICAL(), ensuring that critical sections are properly exited when the kernel is running. The enter/exit logic is now symmetric and correct across all three code paths (ISR, kernel running, and fallback).Drivers/BQ25896/Source/Bq25896.cpp (1)
3-3: LGTM!The include addition is necessary to support the TT_LOG_I logging calls used in this file.
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
36-39: Previous issues have been addressed.The tick-to-milliseconds conversion now correctly multiplies (line 38), and the microseconds-since-boot function no longer truncates the 64-bit value (line 44).
Also applies to: 42-50
Tests/TactilityCore/Main.cpp (1)
51-52: LGTM: Correct handling of scheduler exit.The explicit return ensures that if
vTaskStartScheduler()unexpectedly returns (indicating a scheduler error), the test result is properly propagated instead of implicitly returning 0.TactilityFreeRtos/Include/Tactility/EventGroup.h (2)
108-114: LGTM: Correct ISR-aware implementation.The
get()method correctly handles both ISR and non-ISR contexts to retrieve current event flags.
125-162: LGTM: Well-structured wait implementation.The wait logic correctly handles both "wait for all" and "wait for any" semantics, with appropriate error handling for timeout and resource failures.
TactilityFreeRtos/Include/Tactility/Dispatcher.h (1)
59-85: LGTM: Correct dispatch implementation.The dispatch method properly synchronizes queue access, checks the shutdown flag, and signals consumers. The use of
std::moveon line 72 efficiently transfers the function object.TactilityFreeRtos/Include/Tactility/Thread.h (7)
1-16: LGTM!The includes are well-organized with proper platform-conditional compilation for ESP-specific logging.
52-76: LGTM!The task body correctly manages TLS for thread-local self-pointer, handles state transitions with appropriate assertions, and properly cleans up before task deletion.
128-134: LGTM!The assertion now correctly validates
newStackSizeinstead ofstackSize.
222-237: LGTM!The polling-based join with configurable timeout and poll interval is a practical approach for FreeRTOS. The self-join assertion prevents deadlock.
259-261: LGTM!Named priority constants provide clear semantics for common use cases.
157-157: The_Nullableportability concern is already addressed.The codebase defines
_Nullableas an empty macro across all build configurations via CMake (add_definitions(-D_Nullable=)in multiple CMakeLists.txt files including TactilityFreeRtos, TactilityCore, Tactility, TactilityC, Firmware, and Devices/simulator). This standard pattern ensures the annotation is safely removed on non-Clang compilers (GCC, MSVC) while remaining intact for Clang builds. No changes are needed.Likely an incorrect or invalid review comment.
165-169: The code pattern ingetState()is correct.ScopedLockis designed as a manual-lock, automatic-unlock RAII wrapper: you must explicitly calllock()after construction, andunlock()is called automatically on destruction (documented in Lock.h). Removing thelock.lock()call would leave the mutex unlocked, causing a data race. No changes needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
TactilityFreeRtos/Include/Tactility/Thread.h (2)
115-115: Typo in documentation comment."you just call" should be "you must call". The warning doesn't make grammatical sense as written.
🔎 Proposed fix
- /** @warning If thread is running, you just call join() first */ + /** @warning If thread is running, you must call join() first */
171-182: Race condition: state transition not atomic with precondition checks.The mutex is released at line 176 before
setState(State::Starting)at line 178. This creates a window where:
- Another thread could call
start()and pass thestate == State::Stoppedassertion- State could be observed as
Stoppedwhen it's conceptually transitioningThe lock-unlock-lock pattern (lines 172-176, then 180-182) is also inefficient.
🔎 Proposed fix: keep state transition under single lock
void start() { mutex.lock(); assert(mainFunction); assert(state == State::Stopped); assert(stackSize > 0 && stackSize < (UINT16_MAX * sizeof(StackType_t))); - mutex.unlock(); - - setState(State::Starting); - - mutex.lock(); uint32_t stack_depth = stackSize / sizeof(StackType_t); + state = State::Starting; + StateCallback cb = stateCallback; + void* ctx = stateCallbackContext; mutex.unlock(); + + if (cb) { + cb(State::Starting, ctx); + } BaseType_t result;
🧹 Nitpick comments (2)
TactilityFreeRtos/Include/Tactility/Thread.h (2)
72-73: UseLOCAL_STORAGE_SELF_POINTER_INDEXconstant and protecttaskHandlewrite.Line 72 uses hardcoded
0instead of theLOCAL_STORAGE_SELF_POINTER_INDEXconstant defined at line 20.Additionally,
taskHandle = nullptrat line 73 is written without mutex protection, which can race withgetTaskHandle()calls fromjoin()on another thread.🔎 Proposed fix
- vTaskSetThreadLocalStoragePointer(nullptr, 0, nullptr); - thread->taskHandle = nullptr; + vTaskSetThreadLocalStoragePointer(nullptr, LOCAL_STORAGE_SELF_POINTER_INDEX, nullptr); + thread->mutex.lock(); + thread->taskHandle = nullptr; + thread->mutex.unlock();
90-97: Consider deadlock risk when invoking callback under mutex.The
stateCallbackis invoked while the mutex is held. If the callback attempts to call any Thread method that also acquires the mutex, this will deadlock (if Mutex is non-recursive) or cause unexpected reentrant behavior.Consider either documenting this constraint clearly for users, or invoking the callback outside the lock.
🔎 Alternative: invoke callback outside lock
void setState(State newState) { + StateCallback cb = nullptr; + void* ctx = nullptr; mutex.lock(); state = newState; - if (stateCallback) { - stateCallback(state, stateCallbackContext); - } + cb = stateCallback; + ctx = stateCallbackContext; mutex.unlock(); + if (cb) { + cb(newState, ctx); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/tests.ymlDevices/m5stack-cardputer-adv/Source/devices/CardputerKeyboard.cppDrivers/BQ25896/Source/Bq25896.cppLICENSE.mdTactilityFreeRtos/Include/Tactility/EventGroup.hTactilityFreeRtos/Include/Tactility/Thread.hTactilityFreeRtos/Include/Tactility/kernel/Kernel.h
🚧 Files skipped from review as they are similar to previous changes (3)
- TactilityFreeRtos/Include/Tactility/EventGroup.h
- .github/workflows/tests.yml
- TactilityFreeRtos/Include/Tactility/kernel/Kernel.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Drivers/BQ25896/Source/Bq25896.cpp
📚 Learning: 2025-11-02T15:49:07.895Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 406
File: Buildscripts/CDN/devices.properties:0-0
Timestamp: 2025-11-02T15:49:07.895Z
Learning: In the ByteWelder/Tactility repository, alphabetical ordering issues (such as entries in Buildscripts/CDN/devices.properties) and similar organizational/formatting concerns should be marked as "minor" severity rather than "major" severity.
Applied to files:
LICENSE.md
🧬 Code graph analysis (1)
TactilityFreeRtos/Include/Tactility/Thread.h (1)
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (3)
tt(17-99)getTicks(27-33)delayTicks(70-77)
🪛 LanguageTool
LICENSE.md
[grammar] ~10-~10: Use a hyphen to join words.
Context: ...me](https://kenvanhoeylandt.net). Third party projects that were included in Tac...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: Linux
- GitHub Check: macOS
- GitHub Check: run
🔇 Additional comments (9)
LICENSE.md (1)
1-26: Licensing structure appropriately updated for new TactilityFreeRtos component.The reorganization cleanly separates GPL v3 (main Tactility code) and Apache 2.0 (TactilityC, TactilityFreeRtos, TactilitySDK) licensed components. The FAQ clarifies the build policy for external vs. internal apps. The changes align well with the PR objective to introduce the TactilityFreeRtos subproject.
Drivers/BQ25896/Source/Bq25896.cpp (2)
3-4: LGTM: Logging header added correctly.The include for
Tactility/Log.his necessary for the logging statements used in the power control methods.
5-5: LGTM: TAG correctly set and follows preferred pattern.The TAG is now correctly set to "BQ25896", matching the component name. This resolves the issue from the previous review where it was incorrectly set to "BQ27220". The use of
constexpr auto*follows the codebase preference.Based on learnings,
constexpr autois preferred for constant declarations in the Tactility codebase.Devices/m5stack-cardputer-adv/Source/devices/CardputerKeyboard.cpp (2)
128-130: LGTM! Timer construction follows the new API correctly.The timer is now constructed with an explicit tick-based delay using
pdMS_TO_TICKS(20), which properly converts 20ms to FreeRTOS ticks. This aligns with the broader refactoring to the new FreeRTOS timer integration.
138-138: LGTM! Timer start correctly uses the new parameterless API.The
start()call now relies on the timer's internal delay configured during construction, which is the correct usage pattern for the updated timer API.TactilityFreeRtos/Include/Tactility/Thread.h (4)
121-163: LGTM!Setter methods consistently acquire the mutex, validate preconditions, and release the mutex. The
setStackSizeassertion now correctly validatesnewStackSize.
246-252: LGTM!The
getStackSpace()method correctly guards against ISR context and null task handle before callinguxTaskGetStackHighWaterMark().
259-262: LGTM!Clearly named priority constants improve code readability and consistency across the codebase.
165-169: Remove scoped lock concern—pattern is correct by design.The code pattern
auto lock = mutex.asScopedLock(); lock.lock();is intentional and correct. According to the Lock.h documentation and ScopedLock implementation,asScopedLock()returns an unlocked scoped lock guard that requires manuallock()call. Automaticunlock()is handled by the destructor (RAII). This pattern is consistently used throughout the codebase and is the intended design.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Devices/unphone/Source/UnPhoneFeatures.cpp (1)
40-59: Consider usingstd::atomic<bool>for thread-safe interruption.The
buttonHandlingThreadInterruptRequestflag is written in the destructor (line 63) and read in the thread loop (line 42) without synchronization, creating a data race. While this pattern often works for simple shutdown flags, it's technically undefined behavior.For proper thread safety, consider changing
buttonHandlingThreadInterruptRequesttostd::atomic<bool>.🔎 Proposed refactor to use atomic flag
In the header file (UnPhoneFeatures.h), change the member declaration to:
std::atomic<bool> buttonHandlingThreadInterruptRequest{false};The usage in this file remains the same, but reads and writes will now be atomic operations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Devices/unphone/Source/UnPhoneFeatures.cppLICENSE.mdTactilityC/Source/symbols/freertos.cpp
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-02T15:49:07.895Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 406
File: Buildscripts/CDN/devices.properties:0-0
Timestamp: 2025-11-02T15:49:07.895Z
Learning: In the ByteWelder/Tactility repository, alphabetical ordering issues (such as entries in Buildscripts/CDN/devices.properties) and similar organizational/formatting concerns should be marked as "minor" severity rather than "major" severity.
Applied to files:
LICENSE.md
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Devices/unphone/Source/UnPhoneFeatures.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: run
🔇 Additional comments (4)
LICENSE.md (1)
1-3: Licensing structure clarifications look good.The changes effectively restructure the licensing documentation to clearly distinguish the main GPL v3 license for the core Tactility project from the secondary Apache License 2.0 covering TactilityC, TactilityFreeRtos, and TactilitySDK. The FAQ addition helpfully clarifies the build policy for developers. The past grammatical issue ("Third party" → "Third-party") has been resolved.
Also applies to: 10-10, 12-17, 23-26
Devices/unphone/Source/UnPhoneFeatures.cpp (2)
4-4: LGTM: Include changes improve clarity.The include reordering and explicit addition of
<esp_io_expander.h>are good improvements, making the dependencies clear since this file uses the IO expander APIs extensively.Also applies to: 9-9
102-106: LGTM: Type-safe thread setup.The refactoring to use
setMainFunctionwith a lambda that wrapsbuttonHandlingThreadMainis a clean, type-safe improvement over the previous callback approach. The lambda capture is safe since the thread is properly joined in the destructor.TactilityC/Source/symbols/freertos.cpp (1)
6-6: Remove the unused<Tactility/freertoscompat/RTOS.h>include.The file already includes
<freertos/task.h>and<freertos/event_groups.h>directly, which transitively provide all necessary FreeRTOS declarations. The compatibility header wrapper is not used and can be removed.⛔ Skipped due to learnings
Learnt from: NellowTCS Repo: ByteWelder/Tactility PR: 405 File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12 Timestamp: 2025-11-01T19:57:58.233Z Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Devices/lilygo-tlora-pager/Source/devices/TpagerKeyboard.cpp (1)
93-93: Usett::kernel::millisToTicks()for consistency.Line 93 still uses the old FreeRTOS tick conversion
50 / portTICK_PERIOD_MS, while the timer code (lines 123, 128) now usestt::kernel::millisToTicks(). For consistency with the new API pattern introduced in this PR, consider updating this line as well.🔎 Proposed refactor
- if (chr != '\0') xQueueSend(queue, &chr, 50 / portTICK_PERIOD_MS); + if (chr != '\0') xQueueSend(queue, &chr, tt::kernel::millisToTicks(50));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Devices/lilygo-tlora-pager/Source/devices/TpagerKeyboard.cppTactilityC/Source/symbols/freertos.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
Devices/lilygo-tlora-pager/Source/devices/TpagerKeyboard.cpp (1)
TactilityFreeRtos/Include/Tactility/kernel/Kernel.h (1)
millisToTicks(58-64)
TactilityC/Source/symbols/freertos.cpp (1)
TactilityC/Source/tt_init.cpp (16)
ESP_ELFSYM_EXPORT(69-69)ESP_ELFSYM_EXPORT(70-70)ESP_ELFSYM_EXPORT(71-71)ESP_ELFSYM_EXPORT(72-72)ESP_ELFSYM_EXPORT(73-73)ESP_ELFSYM_EXPORT(74-74)ESP_ELFSYM_EXPORT(75-75)ESP_ELFSYM_EXPORT(76-76)ESP_ELFSYM_EXPORT(78-78)ESP_ELFSYM_EXPORT(79-79)ESP_ELFSYM_EXPORT(81-81)ESP_ELFSYM_EXPORT(83-83)ESP_ELFSYM_EXPORT(84-84)ESP_ELFSYM_EXPORT(85-85)ESP_ELFSYM_EXPORT(86-86)ESP_ELFSYM_EXPORT(87-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Linux
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: macOS
- GitHub Check: run
🔇 Additional comments (6)
Devices/lilygo-tlora-pager/Source/devices/TpagerKeyboard.cpp (4)
2-6: LGTM: Include reordering.The include reorganization improves code organization by grouping related headers together.
123-125: LGTM: Timer construction updated to new API.The timer construction correctly uses the new API with
tt::kernel::millisToTicks(20)for portable tick conversion and explicitType::Periodicspecification.
128-130: LGTM: Timer construction follows the new API pattern.Consistent with the
inputTimerchanges, usingtt::kernel::millisToTicks(50)for portable tick conversion.
138-139: LGTM: Timer start calls updated correctly.The
start()calls correctly omit the period parameter, which is now specified during timer construction.TactilityC/Source/symbols/freertos.cpp (2)
6-6: LGTM: Header path updated correctly.The header path change from
<Tactility/RtosCompat.h>to<Tactility/freertoscompat/RTOS.h>aligns with the PR's introduction of the TactilityFreeRtos component and freertoscompat headers.
11-79: LGTM: Duplicate exports resolved, comprehensive symbol coverage added.The duplicate symbol exports flagged in the previous review have been successfully resolved. Each symbol now appears exactly once in the export table. The additions provide comprehensive coverage of FreeRTOS APIs across Task, EventGroup, Queue, Timer, and portmacro categories, which should properly support the ELF loader/bridge functionality.
No description provided.