TactilityKernel improvements #464
Conversation
📝 WalkthroughWalkthroughAdds a Thread abstraction (header, C++ implementation, tests) and a Timer abstraction (header, C implementation, tests) for concurrent management. Expands I2C controller API: changes read/write/write_read to return 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
KenVanHoeylandt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (51)
build_tests/Libraries/mbedtls/programs/fuzz/Makefile-60-64 (1)
60-64: Avoid committing generated Makefiles with user-specific absolute paths.This file hard-codes
/home/ken/Projects/Tactilityand/home/ken/Projects/Tactility/build_tests, which will break builds on other machines/CI if committed. Prefer excluding generated build artifacts from the repo (and regenerate in CI), or ensure generated files use relative paths/config-time variables instead of user paths.Also applies to: 133-137, 140-143, 161-162, 165-176
build_tests/Libraries/mbedtls/scripts-1-1 (1)
1-1: Replace absolute path with repo-relative content or remove this file.This line hardcodes a developer-specific absolute path, which will break on other machines and likely leaks local environment details. If this file is meant to reference a directory, use a relative path or convert it into a proper script; otherwise remove it.
🛠️ Proposed fix
-/home/ken/Projects/Tactility/Libraries/mbedtls/scripts +# TODO: Replace with a repo-relative path or actual script content.build_tests/Libraries/mbedtls/programs/random/Makefile-1-3 (1)
1-3: Remove generated CMake Makefile from source control.This file is explicitly marked as CMake‑generated and embeds absolute local paths, which makes the repo non‑portable and leaks developer environment details. Please delete this file from the PR and add the build output directory (e.g., build_tests/**) to
.gitignoreso it’s regenerated by CMake instead of committed.build_tests/Libraries/mbedtls/programs/util/Makefile-60-64 (1)
60-64: Avoid committing CMake‑generated Makefiles with absolute, machine‑specific paths.This file hard‑codes
/home/ken/Projects/Tactilityand/usr/bin/cmake, which makes the repo non‑portable and causes churn across developers/CI. Treat these CMake‑generated Makefiles as build artifacts and remove them from version control, adding thebuild_testsoutput tree to.gitignore(or regenerate in CI as needed).Also applies to: 133-143
build_tests/Libraries/mbedtls/CTestTestfile.cmake-1-11 (1)
1-11: Do not commit CMake-generated test artifacts (leaks absolute paths).This file is a build output (note the absolute
/home/ken/...paths) and should be generated at build time, not versioned. Please remove it from the repo and add an ignore rule for build outputs (e.g.,build_tests/or**/CTestTestfile.cmake).🧹 Suggested .gitignore addition (example)
+build_tests/ +# or, if you keep build_tests tracked for some reason: +**/CTestTestfile.cmakebuild_tests/Libraries/mbedtls/programs/util/cmake_install.cmake-45-89 (1)
45-89: Removebuild_testsfrom the repository and add it to.gitignore.The repository commits 41 generated CMake install scripts with absolute hardcoded paths (e.g.,
/home/ken/Projects/Tactility/build_tests/...). These paths are machine-specific and will break installs on any other environment or CI system. Thebuild_testsdirectory contains generated build artifacts (Makefile, cmake_install.cmake, compiled binaries) that should never be committed. Addbuild_tests/to.gitignoreand remove the tracked files viagit rm -r --cached build_tests/.build_tests/Libraries/mbedtls/tests/suites/test_suite_bignum.generated.data-1-1 (1)
1-1: Avoid committing absolute, machine-specific paths.
This hard-coded/home/ken/...path is non-portable and will break on other machines/CI. Prefer relative paths (rooted at repo) or generate these files at build/test time.🔧 Suggested fix (example)
-/home/ken/Projects/Tactility/Libraries/mbedtls/tests/suites/test_suite_bignum.generated.data +Libraries/mbedtls/tests/suites/test_suite_bignum.generated.databuild_tests/Libraries/mbedtls/tests/suites/test_suite_psa_crypto_low_hash.generated.data-1-1 (1)
1-1: Avoid committing absolute build paths into repo artifacts.This hard-coded
/home/ken/...path will break portability and reproducibility on other machines/CI. Prefer generating this file at build time or storing a relative path.✅ Suggested fix (relative path)
-/home/ken/Projects/Tactility/Libraries/mbedtls/tests/suites/test_suite_psa_crypto_low_hash.generated.data +Libraries/mbedtls/tests/suites/test_suite_psa_crypto_low_hash.generated.databuild_tests/Libraries/mbedtls/tests/suites/test_suite_bignum_mod_raw.generated.data-1-1 (1)
1-1: Build artifact with hardcoded absolute path should not be committed.This file contains a machine-specific absolute path (
/home/ken/Projects/...) and appears to be a generated build artifact. Thebuild_tests/directory should be added to.gitignoreto prevent build outputs from being tracked in version control.build_tests/Libraries/mbedtls/DartConfiguration.tcl-1-4 (1)
1-4: CTest-generated configuration file should not be committed.
DartConfiguration.tclis auto-generated by CMake/CTest during the build configuration phase. It contains system-specific tool paths that will differ across environments. This file should be excluded from version control along with other contents ofbuild_tests/.build_tests/Libraries/lvgl/lvgl.pc-1-10 (1)
1-10: Generated pkg-config file should not be committed.This
lvgl.pcfile is a build artifact generated by CMake. It should be created during the build/install process rather than tracked in version control. Thebuild_tests/directory should be gitignored.Minor: Line 3 has inconsistent quoting (
libdir=${prefix}/libvsincludedir="${prefix}/include/lvgl"), but this is moot if the file is removed from version control.build_tests/Tests/TactilityKernel/cmake_install.cmake-1-50 (1)
1-50: Addbuild_tests/to.gitignore— entire directory contains CMake-generated files with hardcoded absolute paths.The
cmake_install.cmakefile and 100+ other auto-generated artifacts inbuild_tests/contain machine-specific absolute paths (/home/ken/Projects/Tactility/...,/usr/bin/objdump) that are non-portable and regenerated on every CMake configuration. These should not be tracked in version control.Required action: Add
build_tests/to.gitignoreto exclude all generated build artifacts.build_tests/Tests/Tactility/CTestTestfile.cmake-2-3 (1)
2-3: CTest file embeds absolute paths.The test executable and source/build directories are hardcoded to
/home/ken/Projects/Tactility(Line 2-3, Line 7-8). This should be generated per-build (not committed) or use CMake variables to remain portable.Also applies to: 7-8
build_tests/Libraries/mbedtls/3rdparty/everest/cmake_install.cmake-1-1 (1)
1-1: Generated install script hardcodes local build paths.The install script uses
/home/ken/Projects/Tactilityfor headers, libraries, and includes (Line 46-54, Line 60-61). This will break in other environments and leaks local paths. Please avoid committing these generated artifacts or parameterize paths via CMake variables.Also applies to: 46-54, 60-61
build_tests/Tests/cmake_install.cmake-1-1 (1)
1-1: Absolute build paths in install script are non-portable.The includes and manifest write target embed
/home/ken/Projects/Tactility(Line 47-63, Line 68-69). These should be generated at build time or use${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CURRENT_SOURCE_DIR}to remain relocatable.Also applies to: 47-63, 68-69
build_tests/Devices/simulator/cmake_install.cmake-1-1 (1)
1-1: Do not commit CMake-generated scripts with absolute user paths.This file embeds
/home/ken/Projects/Tactility(Line 1, Line 48-49), which is non-portable and leaks local paths. These scripts should be generated at build time (and ignored in VCS) or rewritten to use CMake variables like${CMAKE_CURRENT_BINARY_DIR}and${CMAKE_CURRENT_SOURCE_DIR}.Also applies to: 48-49
build_tests/Libraries/SDL/sdl2.pc-3-6 (1)
3-6: Avoid hardcoded install paths in the pkg-config file.Lines 3-6 hardcode
/usr/localandlib64, making the file non-portable. When consumers install SDL to a different prefix, the Libs and Cflags (lines 13-14) will reference incorrect paths. Generate this file from a.pc.intemplate using CMake'sconfigure_file()with@CMAKE_INSTALL_PREFIX@,@CMAKE_INSTALL_LIBDIR@, and@CMAKE_INSTALL_INCLUDEDIR@variables, or usepcfiledir-based relative paths for portability.Also applies to: 13-14
build_tests/Libraries/mbedtls/tests/CTestTestfile.cmake-1-8 (1)
1-8: Generated build artifacts should not be committed to version control.This file is auto-generated by CMake (as indicated by the header comment) and contains hardcoded absolute paths specific to your development machine (
/home/ken/Projects/Tactility/...). These paths will not work for other contributors.The entire
build_tests/directory appears to be a CMake build output directory and should be added to.gitignorerather than committed. Generated files should be excluded from version control to:
- Avoid machine-specific path issues
- Prevent unnecessary merge conflicts
- Keep the repository clean of regeneratable artifacts
build_tests/Platforms/PlatformPosix/cmake_install.cmake-45-49 (1)
45-49: Hard‑coded absolute paths break portability across machines.The install scripts throughout
build_tests/contain hardcoded absolute paths to/home/ken/Projects/Tactility/. These generated cmake_install.cmake files are committed to VCS, making them non-portable—anyone cloning the repository will encounter failing install steps since the paths won't exist on their system.Either exclude
build_tests/from version control (add to.gitignore) or reconfigure the CMake generator to emit build-directory-relative paths instead of absolute paths.build_tests/Tests/TactilityKernel/CTestTestfile.cmake-1-8 (1)
1-8: Avoid committing generated CTest files with absolute paths.Line 2-8 hardcode
/home/ken/Projects/Tactility, which will break on other machines and is regenerated by CMake anyway. Please remove these build artifacts from version control and add the build output directory to.gitignore, keeping them in the build directory only.build_tests/Libraries/SDL/SDL_config.h.intermediate-31-52 (1)
31-52: Removebuild_tests/from source control and add it to.gitignore.The
build_tests/directory contains generated build artifacts (Makefiles, cmake files, .a libraries, .pc package configs). Line 31-52 ofSDL_config.h.intermediatelocks in host-specific configuration flags (HAVE_LIBC,HAVE_GCC_ATOMICS, etc.) that must be generated per target during build. Similar build directories (build/,buildsim/,build-sim/) are already excluded in.gitignore, butbuild_tests/is not. This entire directory should be added to.gitignoreand removed from version control, as it will cause cross-platform builds to inherit incorrect host-specific capabilities.build_tests/TactilityFreeRtos/cmake_install.cmake-1-50 (1)
1-50: CMake-generated install script with hardcoded local paths should not be committed.This is a CMake-generated install script containing hardcoded absolute paths (e.g., line 48:
/home/ken/Projects/Tactility/build_tests/TactilityFreeRtos/...). These paths are specific to your local development environment and will not work for other contributors or CI systems.CMake regenerates these files during the build process, so they should be excluded from version control.
build_tests/Tactility/cmake_install.cmake-1-50 (1)
1-50: CMake-generated install script with hardcoded local paths should not be committed.Same issue as other
cmake_install.cmakefiles in this PR. Line 48 contains a hardcoded path/home/ken/Projects/Tactility/build_tests/Tactility/...that is specific to your local environment.build_tests/Libraries/SDL/cmake_install.cmake-1-50 (1)
1-50: CMake-generated install script with hardcoded local paths should not be committed.Same issue as other
cmake_install.cmakefiles. Line 48 contains hardcoded path/home/ken/Projects/Tactility/build_tests/Libraries/SDL/....Recommendation for the entire PR: The
build_tests/directory appears to be a CMake build output directory that was accidentally committed. Please:
- Remove the entire
build_tests/directory from version control- Add
build_tests/to.gitignore#!/bin/bash # Count how many build artifacts are in this directory echo "=== Files in build_tests/ directory ===" fd -t f . build_tests 2>/dev/null | wc -l echo "" echo "=== Sample of file types ===" fd -t f . build_tests 2>/dev/null | head -20build_tests/Libraries/mbedtls/programs/Makefile-1-189 (1)
1-189: CMake-generated Makefile should not be committed to version control.This file explicitly states "CMAKE generated file: DO NOT EDIT!" (line 1) and contains numerous hardcoded absolute paths specific to your local environment:
- Line 60:
CMAKE_SOURCE_DIR = /home/ken/Projects/Tactility- Line 63:
CMAKE_BINARY_DIR = /home/ken/Projects/Tactility/build_testsCMake regenerates this file during the configure step. Committing it will cause build failures for other developers and CI systems.
build_tests/Libraries/mbedtls/programs/fuzz/cmake_install.cmake-1-50 (1)
1-50: CMake-generated install script with hardcoded local paths should not be committed.Same issue as other
cmake_install.cmakefiles. Line 48 contains hardcoded path/home/ken/Projects/Tactility/build_tests/Libraries/mbedtls/programs/fuzz/....build_tests/Libraries/SDL/include-config-/SDL2/SDL_config.h-1-571 (1)
1-571: Build artifact should not be committed to version control.This file is a CMake-generated configuration header (as indicated by the
SDL_config.h.inreference in the file comment). Thebuild_tests/directory appears to be a build output directory. Committing generated build artifacts causes:
- Unnecessary repository bloat and merge conflicts
- Platform-specific configurations that won't work on other systems
- Confusion between source and generated files
Consider adding
build_tests/to.gitignoreand removing these generated files from version control.#!/bin/bash # Check if build_tests is in .gitignore echo "=== Checking .gitignore for build_tests ===" cat .gitignore 2>/dev/null | grep -E "build|build_tests" || echo "build_tests not found in .gitignore" echo "" echo "=== Checking if this is a build directory ===" fd -t f "CMakeCache.txt" build_tests 2>/dev/null | head -5build_tests/Libraries/mbedtls/programs/aes/Makefile-60-64 (1)
60-64: Generated Makefile embeds developer-specific absolute paths.
CMAKE_SOURCE_DIRandCMAKE_BINARY_DIRare fixed to/home/ken/Projects/Tactility, so runningmakefrom any other checkout will point at the wrong directories. Consider excluding generated build artifacts from version control or regenerating them during build/CI.build_tests/Libraries/mbedtls/programs/pkey/cmake_install.cmake-45-53 (1)
45-53: Install script hard-codes build output paths.The
file(INSTALL ... FILES "/home/ken/Projects/Tactility/..." )entries bake in a single machine’s build directory, which won’t exist elsewhere. If this script is checked in, installs will fail on other systems. Please regenerate these scripts per build or derive paths from${CMAKE_CURRENT_LIST_DIR}/${CMAKE_BINARY_DIR}instead.build_tests/Libraries/minmea/Makefile-60-64 (1)
60-64: Generated Makefile embeds absolute paths.The hard-coded
CMAKE_SOURCE_DIR/CMAKE_BINARY_DIRvalues will break builds outside/home/ken/Projects/Tactility. Please avoid committing generated Makefiles or ensure they’re regenerated in each environment.build_tests/Libraries/mbedtls/programs/aes/cmake_install.cmake-45-53 (1)
45-53: Install script hard-codes build output paths.The
file(INSTALL ...)paths are absolute to one developer machine. If this file is committed, installs will fail for anyone whose build directory differs. Prefer regenerating these scripts per build or deriving paths from${CMAKE_CURRENT_LIST_DIR}/${CMAKE_BINARY_DIR}.build_tests/Libraries/SDL/cmake_uninstall.cmake-1-10 (1)
1-10: Make the uninstall script path-agnostic (avoid hard-coded absolute paths).The manifest path and CMake executable are fixed to a single machine, so uninstall will fail when the repo is checked out elsewhere or CMake isn’t at
/usr/bin/cmake. Derive the manifest from the script location and use${CMAKE_COMMAND}instead.Proposed fix
-if (NOT EXISTS "/home/ken/Projects/Tactility/build_tests/install_manifest.txt") - message(FATAL_ERROR "Cannot find install manifest: \"/home/ken/Projects/Tactility/build_tests/install_manifest.txt\"") -endif(NOT EXISTS "/home/ken/Projects/Tactility/build_tests/install_manifest.txt") - -file(READ "/home/ken/Projects/Tactility/build_tests/install_manifest.txt" files) +get_filename_component(_uninstall_root "${CMAKE_CURRENT_LIST_DIR}/../.." ABSOLUTE) +set(_manifest "${_uninstall_root}/install_manifest.txt") +if (NOT EXISTS "${_manifest}") + message(FATAL_ERROR "Cannot find install manifest: \"${_manifest}\"") +endif() + +file(READ "${_manifest}" files) @@ - COMMAND /usr/bin/cmake -E remove "$ENV{DESTDIR}${file}" + COMMAND "${CMAKE_COMMAND}" -E remove "$ENV{DESTDIR}${file}"build_tests/Libraries/FreeRTOS-Kernel/portable/cmake_install.cmake-45-49 (1)
45-49: Write the local manifest relative to the script directory.The absolute path ties the script to one checkout and breaks on other machines. Use
${CMAKE_CURRENT_LIST_DIR}(or the current binary dir) so it stays portable.Proposed fix
-if(CMAKE_INSTALL_LOCAL_ONLY) - file(WRITE "/home/ken/Projects/Tactility/build_tests/Libraries/FreeRTOS-Kernel/portable/install_local_manifest.txt" - "${CMAKE_INSTALL_MANIFEST_CONTENT}") -endif() +if(CMAKE_INSTALL_LOCAL_ONLY) + file(WRITE "${CMAKE_CURRENT_LIST_DIR}/install_local_manifest.txt" + "${CMAKE_INSTALL_MANIFEST_CONTENT}") +endif()build_tests/Libraries/minitar/cmake_install.cmake-1-49 (1)
1-49: Addbuild_tests/to.gitignoreor remove committed build artifacts.The
build_tests/directory contains CMake-generated files with hardcoded machine-local paths (/home/ken/Projects/Tactility/...), but is not in.gitignore. This appears across multiple files:cmake_install.cmake,Makefile, andCTestTestfile.cmakein subdirectories likeTests/,TactilityKernel/,TactilityFreeRtos/, and more. These are generated build artifacts that should be excluded from version control. Addbuild_tests/to.gitignore, then remove these files from the repository.build_tests/Tests/TactilityFreeRtos/Makefile-59-64 (1)
59-64: Generated Makefile hardcodes absolute build pathsLine 60-64 embed
/home/ken/Projects/Tactility. If this Makefile is committed, builds on other machines will break. Please avoid committing generated build files or regenerate per build with local paths.build_tests/Libraries/lv_screenshot/cmake_install.cmake-45-49 (1)
45-49: Avoid hard-coded absolute manifest pathLine 48-49 writes to
/home/ken/Projects/..., which will fail on other machines if this script is committed. Use a path relative to the script location (or regenerate per build).Suggested fix
- file(WRITE "/home/ken/Projects/Tactility/build_tests/Libraries/lv_screenshot/install_local_manifest.txt" + file(WRITE "${CMAKE_CURRENT_LIST_DIR}/install_local_manifest.txt" "${CMAKE_INSTALL_MANIFEST_CONTENT}")build_tests/Tests/TactilityFreeRtos/cmake_install.cmake-1-49 (1)
1-49: Remove machine-specific absolute paths from checked-in build artifacts.Lines 1 and 48 hardcode
/home/ken/Projects/Tactility/..., which will break builds and installations on other machines. Sincebuild_tests/is tracked in git, this file will be distributed to all clones with a non-existent path. Either regeneratecmake_install.cmakeper-build (excludebuild_tests/from VCS), or configure CMake to use relative paths (e.g.,${CMAKE_BINARY_DIR}-relative paths).build_tests/Libraries/SDL/sdl2-config-8-58 (1)
8-58: Split--libsand--static-libsbehavior; synclibdirwith prefix changes.The current code violates official SDL2 behavior:
--libsshould return shared library flags (e.g.,-lSDL2), but currently returns static paths--static-libsshould return static library flags with private dependenciesAdditionally,
libdiris initialized once (line 10) but never refreshed when--prefixor--exec-prefixare set (lines 29–40), causing stale paths.Proposed fix
exec_prefix=${prefix} exec_prefix_set=no libdir=${exec_prefix}/lib64 --prefix=*) prefix=$optarg if test $exec_prefix_set = no ; then exec_prefix=$optarg fi + libdir=${exec_prefix}/lib64 ;; --exec-prefix=*) exec_prefix=$optarg exec_prefix_set=yes + libdir=${exec_prefix}/lib64 ;; - --libs|--static-libs) - sdl_static_libs=$(echo " -lSDL2 -pthread -lm" | sed -E "s#-lSDL2[ $]#$libdir/libSDL2.a `#g`") - echo -L${exec_prefix}/lib64 $sdl_static_libs - ;; + --libs) + echo -L${exec_prefix}/lib64 -lSDL2 -pthread -lm + ;; + --static-libs) + sdl_static_libs=$(echo " -lSDL2 -pthread -lm" | sed -E "s#-lSDL2[ $]#$libdir/libSDL2.a `#g`") + echo -L${exec_prefix}/lib64 $sdl_static_libs + ;;build_tests/Libraries/lv_screenshot/Makefile-1-63 (1)
1-63: Generated build artifacts should not be committed to version control.This file is auto-generated by CMake (as indicated by line 1: "CMAKE generated file: DO NOT EDIT!") and contains hardcoded absolute paths specific to the developer's machine (e.g.,
/home/ken/Projects/Tactility).These files:
- Won't work on other developers' machines due to hardcoded paths
- Will be regenerated when running
cmake- Unnecessarily bloat the repository
Consider adding
build_tests/to.gitignoreand removing these generated files from version control.build_tests/Firmware/cmake_install.cmake-1-50 (1)
1-50: Addbuild_tests/to.gitignoreto prevent committing CMake build artifacts.The
build_tests/directory contains CMake-generated files (Makefiles, cmake_install.cmake, generated headers, libraries) with hardcoded machine-specific paths (e.g.,/home/ken/Projects/Tactility/). This directory is a build output folder and should be excluded from version control like the existingbuild/,buildsim/, andbuild-sim/entries in.gitignore. These generated artifacts will break builds for other developers and are regenerated during CMake configuration.build_tests/Devices/simulator/Makefile-60-64 (1)
60-64: Generated Makefile with absolute paths is not portable.This file embeds
/home/ken/Projects/Tactility, which will break builds on other machines and CI. If this is generated, it should be excluded from version control (or regenerated as part of build), not committed.build_tests/Libraries/mbedtls/include/psa-1-1 (1)
1-1: Absolute-path sentinel file should not be committed.This single-line file hardcodes a developer-specific path. It should be generated at build time or replaced with a portable include-path configuration, not checked into the repo.
build_tests/Libraries/mbedtls/include/mbedtls-1-1 (1)
1-1: Absolute-path sentinel file should not be committed.This file hardcodes a local path and will fail in other environments. Please generate this during the build or replace it with portable configuration.
TactilityKernel/Source/concurrent/thread.cpp-162-210 (1)
162-210: Reset state when task creation fails.If
xTaskCreate*fails, the thread remains inTHREAD_STATE_STARTING, which prevents reconfiguration andthread_free. Roll back to STOPPED and clear the handle on failure.🛠️ Proposed fix
- return (result == pdPASS) ? ERROR_NONE : ERROR_UNDEFINED; + if (result != pdPASS) { + thread_set_state_internal(thread, THREAD_STATE_STOPPED); + thread->lock(); + thread->taskHandle = nullptr; + thread->unlock(); + return ERROR_UNDEFINED; + } + return ERROR_NONE;TactilityKernel/Source/concurrent/thread.cpp-42-48 (1)
42-48: Avoid invoking state callbacks while holding the mutex.Calling user callbacks under the lock risks deadlocks/re-entrancy and extends the critical section. Capture the callback/context under the lock, then invoke after unlocking.
🛠️ Proposed fix
static void thread_set_state_internal(Thread* thread, ThreadState newState) { - thread->lock(); - thread->state = newState; - if (thread->stateCallback) { - thread->stateCallback(thread->state, thread->stateCallbackContext); - } - thread->unlock(); + thread_state_callback_t callback = nullptr; + void* context = nullptr; + thread->lock(); + thread->state = newState; + callback = thread->stateCallback; + context = thread->stateCallbackContext; + thread->unlock(); + if (callback) { + callback(newState, context); + } }TactilityKernel/Source/concurrent/thread.cpp-63-71 (1)
63-71: GuardcallbackResultandtaskHandleupdates with the mutex.These fields are read under lock elsewhere; writing them without the lock breaks the synchronization contract and can race with readers.
🛠️ Proposed fix
- thread->callbackResult = thread->mainFunction(thread->mainFunctionContext); + const int32_t result = thread->mainFunction(thread->mainFunctionContext); + thread->lock(); + thread->callbackResult = result; + thread->unlock(); ... - thread->taskHandle = nullptr; + thread->lock(); + thread->taskHandle = nullptr; + thread->unlock();build_tests/Tests/CTestTestfile.cmake-1-10 (1)
1-10: Remove auto-generated CMake build artifacts from version control.This file is auto-generated by CMake (as stated in Line 1: "CMake generated Testfile") and contains hardcoded absolute paths specific to your machine (
/home/ken/Projects/Tactility/...). Build output directories likebuild_tests/should be added to.gitignorerather than committed.These files will cause issues for other contributors since the paths won't match their environments, and they'll be overwritten on each CMake configuration anyway.
build_tests/Tests/Makefile-1-63 (1)
1-63: Addbuild_tests/to.gitignoreto exclude CMake-generated build artifacts.This file is auto-generated by CMake (as indicated on line 1) and contains hardcoded absolute paths specific to the developer's machine (e.g.,
/home/ken/Projects/Tactility). Thebuild_tests/directory contains 101 generated Makefile and build files that should not be version-controlled.While similar build directories (
build/,buildsim/,build-sim/,cmake-build-*/) are already excluded in.gitignore, thebuild_tests/pattern is missing. Add it to ensure consistency and prevent generated artifacts from being committed.build_tests/cmake_install.cmake-1-151 (1)
1-151: Generated build artifact should not be committed.This CMake-generated install script contains hardcoded absolute paths (e.g.,
/home/ken/Projects/Tactility/...) that are specific to the local development machine. This file:
- Will not work on other machines or CI environments
- Should be in
.gitignore- Is regenerated by CMake during build configuration
Consider removing this file from version control and adding
build_tests/to.gitignore.build_tests/Libraries/mbedtls/3rdparty/everest/Makefile-1-285 (1)
1-285: Generated build artifact should not be committed.This is a CMake-generated Makefile (as noted in line 1: "CMAKE generated file: DO NOT EDIT!") containing hardcoded absolute paths specific to the local development environment. This file should be excluded from version control.
The static analysis warnings about missing
.PHONYtargets are false positives - the targetsall(line 138),clean(line 143), andeverest(line 171) are properly declared as.PHONY.Consider adding
build_tests/to.gitignoreto prevent generated build artifacts from being committed.TactilityKernel/Source/timer.c-44-50 (1)
44-50: Fix use-after-free race condition intimer_free.
xTimerDeleteis asynchronous—it only enqueues a delete command to the FreeRTOS timer service task; the actual timer deletion happens later. If the timer callback (timer_callback_internalat line 14) is executing or scheduled on the timer daemon task whenfree(timer)is called at line 49, the callback's accesses totimer->callbackandtimer->context(line 17) will read from freed memory.The NULL check at line 16 does not prevent use-after-free; it only guards against null pointer dereference on an already-freed pointer.
Either defer
free(timer)until the delete command has been processed by the timer service task, or document that the caller must stop the timer and ensure no callback is in progress before callingtimer_free.
🟡 Minor comments (6)
CODING_STYLE_C.md-102-108 (1)
102-108: Typo: "succesful" should be "successful".Line 106 contains a spelling error in the documentation example.
📝 Proposed fix
/** * `@brief` Validates a number * `@param`[in] number the integer to validate - * `@return` true if validation was succesful and there were no issues + * `@return` true if validation was successful and there were no issues */ bool validate(int number);CODING_STYLE_C.md-110-116 (1)
110-116: Incorrect Doxygen@paramsyntax and naming convention inconsistency.Two issues in this example:
Line 112: The
@paramsyntax is incorrect. The direction specifier[in]should come before the parameter name, not after:@param[in] timeoutinstead of@param timeout[in].Line 116: The function name
runActionuses camelCase, but the coding style document specifies that function names should use lower_snake_case (see "Function names" section above).📝 Proposed fix
/** * `@brief` Run the action. - * `@param` timeout[in] the maximum time the task should run + * `@param`[in] timeout the maximum time the task should run * `@retval` ERROR_TIMEOUT when the task couldn't be completed on time * `@retval` ERROR_NONE when the task completed successfully */ -error_t runAction(TickType_t timeout); +error_t run_action(TickType_t timeout);build_tests/Libraries/mbedtls/programs/psa/Makefile-59-64 (1)
59-64: Avoid committing generated Makefiles with absolute local paths.Line 59-64 hard-codes
/home/ken/Projects/Tactility, which breaks portability and leaks local paths if these artifacts are committed. Consider excluding generated build outputs from VCS or ensuring the build system regenerates them per-environment.build_tests/Libraries/SDL/SDL2.spec-5-6 (1)
5-6: Use HTTPS for SDL2 source and URL downloads.Lines 5-6 currently use HTTP for both the source tarball URL and the project URL. SDL2's official releases are available over HTTPS at
https://www.libsdl.org/release/, which provides better protection against man-in-the-middle attacks during source retrieval.🔧 Proposed fix
-Source: http://www.libsdl.org/release/%{name}-%{version}.tar.gz -URL: http://www.libsdl.org/ +Source: https://www.libsdl.org/release/%{name}-%{version}.tar.gz +URL: https://www.libsdl.org/TactilityKernel/Include/tactility/concurrent/thread.h-4-4 (1)
4-4: Align include style with codebase convention (use angle brackets for tactility headers).Other headers in the concurrent directory use
<tactility/error.h>for includes from the tactility project. Adopt this style for consistency.Include style alignment
-#include "tactility/error.h" +#include <tactility/error.h>Tests/TactilityKernel/ThreadTest.cpp-7-47 (1)
7-47: Use atomic flags for cross-thread signaling in these tests.Plain
boolvariables captured by reference and accessed from threads create data races. Thewhile (!interrupted)loop in particular reads an unprotected flag from the worker thread. Usestd::atomic<bool>instead, following the pattern already established inTimerTest.cpp.🛠️ Example fix
`#include` "doctest.h" `#include` "tactility/delay.h" `#include` <tactility/concurrent/thread.h> +#include <atomic> using namespace tt; TEST_CASE("when a thread is started then its callback should be called") { - bool has_called = false; + std::atomic<bool> has_called{false}; auto* thread = new Thread( "immediate return task", 4096, [&has_called]() { - has_called = true; + has_called.store(true, std::memory_order_release); return 0; } ); - CHECK(!has_called); + CHECK(!has_called.load(std::memory_order_acquire)); thread->start(); thread->join(); delete thread; - CHECK(has_called); + CHECK(has_called.load(std::memory_order_acquire)); } TEST_CASE("a thread can be started and stopped") { - bool interrupted = false; + std::atomic<bool> interrupted{false}; auto* thread = new Thread( "interruptable thread", 4096, [&interrupted]() { - while (!interrupted) { + while (!interrupted.load(std::memory_order_acquire)) { kernel::delayMillis(5); } return 0; } ); CHECK(thread); thread->start(); - interrupted = true; + interrupted.store(true, std::memory_order_release); thread->join(); delete thread; }Apply the same pattern to the remaining test cases (lines 45+, 66+) that also use plain
bool interrupted.
🧹 Nitpick comments (4)
TactilityKernel/Include/tactility/timer.h (1)
84-100: Unusedtimerparameter intimer_set_pending_callbackandtimer_set_callback_priority.Per the implementation in
TactilityKernel/Source/timer.c, thetimerparameter is marked as unused with(void)timer. The documentation doesn't indicate this is a reserved parameter for future use. Consider either:
- Documenting that the parameter is currently unused but reserved for future API extensions, or
- Removing the parameter if it's not anticipated to be needed
This is a minor API clarity concern — the current approach is reasonable if the intent is future-proofing.
TactilityKernel/Source/timer.c (3)
64-71: RedundantxTimerResetcall afterxTimerChangePeriod.
xTimerChangePeriodalready starts/restarts the timer with the new period. The subsequentxTimerResetcall is redundant and causes an unnecessary command to the timer queue.♻️ Suggested simplification
error_t timer_reset_with_interval(struct Timer* timer, TickType_t interval) { check(xPortInIsrContext() == pdFALSE); check(timer != NULL); - if (xTimerChangePeriod(timer->handle, interval, portMAX_DELAY) != pdPASS) { - return ERROR_TIMEOUT; - } - return (xTimerReset(timer->handle, portMAX_DELAY) == pdPASS) ? ERROR_NONE : ERROR_TIMEOUT; + return (xTimerChangePeriod(timer->handle, interval, portMAX_DELAY) == pdPASS) ? ERROR_NONE : ERROR_TIMEOUT; }
91-101: ISR path doesn't signal context switch when higher-priority task is woken.Line 96 passes
NULLforpxHigherPriorityTaskWoken. If the pended callback wakes a higher-priority task, no context switch will occur until the next tick, potentially causing latency.Consider capturing and returning the flag to allow the caller to yield if needed, or document this limitation.
103-110: Misleading API:timerparameter is unused and function affects all timers.This function changes the priority of the global timer daemon task, affecting the callback priority of all timers in the system, not just the one passed in. The unused
timerparameter is misleading.Consider either:
- Renaming to
timer_daemon_set_priority(enum ThreadPriority priority)to clarify the global effect- Adding documentation warning that this affects all timer callbacks system-wide
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
TactilityKernel/Include/tactility/drivers/gpio_controller.h (1)
86-94: Naming inconsistency: Consider renaming togpio_controller_set_options_config.All other functions in this header follow the
gpio_controller_*naming convention. This inline helper breaks that pattern.Additionally, consider whether a NULL check for
configis warranted for defensive programming, though this may be intentionally left to the caller.♻️ Suggested rename for consistency
-static inline error_t gpio_set_options_config(struct Device* device, const struct GpioPinConfig* config) { +static inline error_t gpio_controller_set_options_config(struct Device* device, const struct GpioPinConfig* config) { return gpio_controller_set_options(device, config->pin, config->flags); }TactilityKernel/Include/tactility/drivers/i2c_controller.h (1)
30-30: Consider unifyingdataSizetypes for consistency.
readusessize_tfordataSizewhilewriteusesuint16_t. This inconsistency could cause confusion and potential truncation issues if callers expect uniform types.Also applies to: 42-42
TactilityKernel/Source/drivers/i2c_controller.cpp (1)
46-50: Use dedicated I2C probing API instead of generic write.The current 2-byte write approach is non-standard for I2C device detection. While address-only probing (START + address + STOP) is the correct pattern, passing zero-length writes via the generic
write()API is undocumented and unreliable across ESP-IDF versions—it may be treated as a no-op or invalid argument.For proper device probing on ESP-IDF v5, use
i2c_master_probe(bus_handle, addr, timeout_ms). On v4.x, construct an explicit command link with START/address/STOP. Avoid relying on zero-length writes for probe semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Platforms/PlatformEsp32/Source/drivers/esp32_i2c.cpp (1)
56-64: Missing zero-size validation for consistency.Unlike
read,write,read_register, andwrite_register, this function doesn't validate thatwrite_data_sizeandread_data_sizeare non-zero. Consider adding validation for consistency:Proposed fix
static error_t write_read(Device* device, uint8_t address, const uint8_t* write_data, size_t write_data_size, uint8_t* read_data, size_t read_data_size, TickType_t timeout) { if (xPortInIsrContext()) return ERROR_ISR_STATUS; + if (write_data_size == 0 || read_data_size == 0) return ERROR_INVALID_ARGUMENT; auto* driver_data = GET_DATA(device);
🧹 Nitpick comments (1)
TactilityKernel/Source/concurrent/thread.cpp (1)
258-267: Inconsistent use ofassertvscheckfor state validation.Line 263 uses
assert()instead ofcheck(), which is inconsistent with the rest of the file and may be compiled out in release builds, leaving the state check unprotected in production.🔧 Proposed fix
uint32_t thread_get_stack_space(Thread* thread) { if (xPortInIsrContext() == pdTRUE) { return 0; } thread->lock(); - assert(thread->state == THREAD_STATE_RUNNING); + check(thread->state == THREAD_STATE_RUNNING); auto result = uxTaskGetStackHighWaterMark(thread->taskHandle) * sizeof(StackType_t); thread->unlock(); return result; }
| i2c_cmd_handle_t cmd = i2c_cmd_link_create(); | ||
| // Set address pointer | ||
| i2c_master_start(cmd); | ||
| i2c_master_write_byte(cmd, (address << 1) | I2C_MASTER_WRITE, ACK_CHECK_EN); | ||
| i2c_master_write(cmd, ®, 1, ACK_CHECK_EN); | ||
| // Read length of response from current pointer | ||
| i2c_master_start(cmd); | ||
| i2c_master_write_byte(cmd, (address << 1) | I2C_MASTER_READ, ACK_CHECK_EN); | ||
| if (data_size > 1) { | ||
| i2c_master_read(cmd, data, data_size - 1, I2C_MASTER_ACK); | ||
| } | ||
| i2c_master_read_byte(cmd, data + data_size - 1, I2C_MASTER_NACK); | ||
| i2c_master_stop(cmd); | ||
| // TODO: We're passing an inaccurate timeout value as we already lost time with locking | ||
| esp_err_t esp_error = i2c_master_cmd_begin(GET_CONFIG(device)->port, cmd, timeout); | ||
| i2c_cmd_link_delete(cmd); |
There was a problem hiding this comment.
Missing NULL check for i2c_cmd_link_create() return value.
i2c_cmd_link_create() can return NULL if memory allocation fails. Subsequent i2c_master_* calls on a NULL handle could cause undefined behavior or crashes under memory pressure.
Proposed fix
lock(driver_data);
i2c_cmd_handle_t cmd = i2c_cmd_link_create();
+ if (cmd == NULL) {
+ unlock(driver_data);
+ return ERROR_OUT_OF_MEMORY;
+ }
// Set address pointer
i2c_master_start(cmd);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| i2c_cmd_handle_t cmd = i2c_cmd_link_create(); | |
| // Set address pointer | |
| i2c_master_start(cmd); | |
| i2c_master_write_byte(cmd, (address << 1) | I2C_MASTER_WRITE, ACK_CHECK_EN); | |
| i2c_master_write(cmd, ®, 1, ACK_CHECK_EN); | |
| // Read length of response from current pointer | |
| i2c_master_start(cmd); | |
| i2c_master_write_byte(cmd, (address << 1) | I2C_MASTER_READ, ACK_CHECK_EN); | |
| if (data_size > 1) { | |
| i2c_master_read(cmd, data, data_size - 1, I2C_MASTER_ACK); | |
| } | |
| i2c_master_read_byte(cmd, data + data_size - 1, I2C_MASTER_NACK); | |
| i2c_master_stop(cmd); | |
| // TODO: We're passing an inaccurate timeout value as we already lost time with locking | |
| esp_err_t esp_error = i2c_master_cmd_begin(GET_CONFIG(device)->port, cmd, timeout); | |
| i2c_cmd_link_delete(cmd); | |
| i2c_cmd_handle_t cmd = i2c_cmd_link_create(); | |
| if (cmd == NULL) { | |
| unlock(driver_data); | |
| return ERROR_OUT_OF_MEMORY; | |
| } | |
| // Set address pointer | |
| i2c_master_start(cmd); | |
| i2c_master_write_byte(cmd, (address << 1) | I2C_MASTER_WRITE, ACK_CHECK_EN); | |
| i2c_master_write(cmd, ®, 1, ACK_CHECK_EN); | |
| // Read length of response from current pointer | |
| i2c_master_start(cmd); | |
| i2c_master_write_byte(cmd, (address << 1) | I2C_MASTER_READ, ACK_CHECK_EN); | |
| if (data_size > 1) { | |
| i2c_master_read(cmd, data, data_size - 1, I2C_MASTER_ACK); | |
| } | |
| i2c_master_read_byte(cmd, data + data_size - 1, I2C_MASTER_NACK); | |
| i2c_master_stop(cmd); | |
| // TODO: We're passing an inaccurate timeout value as we already lost time with locking | |
| esp_err_t esp_error = i2c_master_cmd_begin(GET_CONFIG(device)->port, cmd, timeout); | |
| i2c_cmd_link_delete(cmd); |
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.