Skip to content

Add drivers to module and make start/stop optional#580

Open
KenVanHoeylandt wants to merge 2 commits into
mainfrom
module-drivers
Open

Add drivers to module and make start/stop optional#580
KenVanHoeylandt wants to merge 2 commits into
mainfrom
module-drivers

Conversation

@KenVanHoeylandt

@KenVanHoeylandt KenVanHoeylandt commented Jul 22, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features
    • Added automatic driver registration for supported devices and modules via declarative driver lists.
    • Improved module startup/shutdown handling.
    • Disabled use of the deprecated HAL on selected LilyGo devices.
  • Bug Fixes
    • Improved power-off behavior for a supported display device.
  • Documentation
    • Updated the audio codec module documentation.
    • Added a planning note for audio codec module organization.
  • Chores
    • Removed the deprecated ESP-LCD compatibility layer implementation and its documentation section.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 31fb5cb6-4a80-4e3a-8055-f18dd68b9e50

📥 Commits

Reviewing files that changed from the base of the PR and between 9b020ff and 74bcd97.

📒 Files selected for processing (64)
  • Devices/m5stack-papers3/source/module.cpp
  • Devices/m5stack-tab5/Source/module.cpp
  • Devices/unphone/source/module.cpp
  • Drivers/aw88298-module/source/module.cpp
  • Drivers/aw9523b-module/source/module.cpp
  • Drivers/axp192-module/source/module.cpp
  • Drivers/axp2101-module/source/module.cpp
  • Drivers/axs5106-module/source/module.cpp
  • Drivers/bm8563-module/source/module.cpp
  • Drivers/bmi270-module/source/module.cpp
  • Drivers/bq24295-module/source/module.cpp
  • Drivers/bq25896-module/source/module.cpp
  • Drivers/bq27220-module/source/module.cpp
  • Drivers/button-control-module/source/module.cpp
  • Drivers/ch422g-module/source/module.cpp
  • Drivers/cst328-module/source/module.cpp
  • Drivers/cst66xx-module/source/module.cpp
  • Drivers/cst816s-module/source/module.cpp
  • Drivers/cst816t-module/source/module.cpp
  • Drivers/drv2605-module/source/module.cpp
  • Drivers/dummy-i2s-amp-module/source/module.cpp
  • Drivers/es7210-module/source/module.cpp
  • Drivers/es8311-module/source/module.cpp
  • Drivers/es8388-module/source/module.cpp
  • Drivers/ft5x06-module/source/module.cpp
  • Drivers/ft6x36-module/source/module.cpp
  • Drivers/gc9a01-module/source/module.cpp
  • Drivers/gt911-module/source/module.cpp
  • Drivers/hx8357-module/source/module.cpp
  • Drivers/ili9341-module/source/module.cpp
  • Drivers/ili9488-module/source/module.cpp
  • Drivers/ili9881c-module/source/module.cpp
  • Drivers/ina226-module/source/module.cpp
  • Drivers/jd9165-module/source/module.cpp
  • Drivers/jd9853-module/source/module.cpp
  • Drivers/lilygo-module/source/module.cpp
  • Drivers/m5pm1-module/source/module.cpp
  • Drivers/m5stack-module/source/module.cpp
  • Drivers/mpu6886-module/source/module.cpp
  • Drivers/pdm-mic-module/source/module.cpp
  • Drivers/pi4ioe5v6408-module/source/module.cpp
  • Drivers/py32ioexpander-module/source/module.cpp
  • Drivers/qmi8658-module/source/module.cpp
  • Drivers/rgb-display-module/source/module.cpp
  • Drivers/rx8130ce-module/source/module.cpp
  • Drivers/sc2356-module/source/module.cpp
  • Drivers/ssd1306-module/source/module.cpp
  • Drivers/st7123-module/source/module.cpp
  • Drivers/st7701-module/source/module.cpp
  • Drivers/st7735-module/source/module.cpp
  • Drivers/st7789-i8080-module/source/module.cpp
  • Drivers/st7789-module/source/module.cpp
  • Drivers/st7796-i8080-module/source/module.cpp
  • Drivers/st7796-module/source/module.cpp
  • Drivers/sy6970-module/source/module.cpp
  • Drivers/tca8418-module/source/module.cpp
  • Drivers/tca9534-module/source/module.cpp
  • Drivers/tca95xx-16bit-module/source/module.cpp
  • Drivers/xl9555-module/source/module.cpp
  • Drivers/xpt2046-module/source/module.cpp
  • Drivers/xpt2046-softspi-module/source/module.cpp
  • Modules/hal-device-module/source/module.cpp
  • TactilityKernel/include/tactility/module.h
  • TactilityKernel/source/module.cpp
🚧 Files skipped from review as they are similar to previous changes (63)
  • Drivers/ft6x36-module/source/module.cpp
  • Drivers/cst328-module/source/module.cpp
  • Drivers/ili9881c-module/source/module.cpp
  • Drivers/ili9488-module/source/module.cpp
  • Drivers/gt911-module/source/module.cpp
  • Drivers/bmi270-module/source/module.cpp
  • Drivers/axs5106-module/source/module.cpp
  • Drivers/ili9341-module/source/module.cpp
  • Drivers/rx8130ce-module/source/module.cpp
  • Drivers/cst66xx-module/source/module.cpp
  • Drivers/pi4ioe5v6408-module/source/module.cpp
  • Drivers/ssd1306-module/source/module.cpp
  • Drivers/bm8563-module/source/module.cpp
  • Drivers/aw88298-module/source/module.cpp
  • Drivers/st7735-module/source/module.cpp
  • Drivers/jd9165-module/source/module.cpp
  • Drivers/tca95xx-16bit-module/source/module.cpp
  • Drivers/es7210-module/source/module.cpp
  • Drivers/st7701-module/source/module.cpp
  • Drivers/qmi8658-module/source/module.cpp
  • Devices/m5stack-papers3/source/module.cpp
  • Drivers/drv2605-module/source/module.cpp
  • Drivers/cst816t-module/source/module.cpp
  • Drivers/xl9555-module/source/module.cpp
  • Drivers/dummy-i2s-amp-module/source/module.cpp
  • Drivers/es8311-module/source/module.cpp
  • Drivers/st7789-module/source/module.cpp
  • Drivers/tca9534-module/source/module.cpp
  • Drivers/pdm-mic-module/source/module.cpp
  • Drivers/es8388-module/source/module.cpp
  • Drivers/cst816s-module/source/module.cpp
  • Drivers/axp2101-module/source/module.cpp
  • Drivers/ft5x06-module/source/module.cpp
  • Drivers/st7796-module/source/module.cpp
  • Drivers/button-control-module/source/module.cpp
  • Drivers/gc9a01-module/source/module.cpp
  • Drivers/ch422g-module/source/module.cpp
  • Drivers/bq24295-module/source/module.cpp
  • Drivers/xpt2046-softspi-module/source/module.cpp
  • Drivers/aw9523b-module/source/module.cpp
  • Drivers/py32ioexpander-module/source/module.cpp
  • Drivers/sc2356-module/source/module.cpp
  • Drivers/xpt2046-module/source/module.cpp
  • Drivers/sy6970-module/source/module.cpp
  • Drivers/ina226-module/source/module.cpp
  • Devices/m5stack-tab5/Source/module.cpp
  • Drivers/bq27220-module/source/module.cpp
  • Drivers/rgb-display-module/source/module.cpp
  • Drivers/st7123-module/source/module.cpp
  • Drivers/hx8357-module/source/module.cpp
  • Drivers/st7789-i8080-module/source/module.cpp
  • Drivers/m5stack-module/source/module.cpp
  • Drivers/bq25896-module/source/module.cpp
  • Drivers/st7796-i8080-module/source/module.cpp
  • Drivers/tca8418-module/source/module.cpp
  • TactilityKernel/include/tactility/module.h
  • Drivers/lilygo-module/source/module.cpp
  • Drivers/jd9853-module/source/module.cpp
  • Drivers/m5pm1-module/source/module.cpp
  • TactilityKernel/source/module.cpp
  • Modules/hal-device-module/source/module.cpp
  • Devices/unphone/source/module.cpp
  • Drivers/axp192-module/source/module.cpp
📝 Walkthrough

Walkthrough

The module API now accepts optional lifecycle callbacks and null-terminated driver lists. Kernel start and stop operations construct, add, remove, and destruct registered drivers around optional callbacks. Device and driver modules were migrated from explicit lifecycle wiring to declarative driver arrays, while several device descriptors now use minimal initializers. Additional changes remove obsolete placeholders and headers, update selected device HAL properties, adjust power and event cleanup behavior, and revise related documentation.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: modules now support drivers and start/stop hooks are optional.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch module-drivers

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot reviewed Jul 22, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 310d2eb2-eba3-4769-8fea-f7e9cbc3886b

📥 Commits

Reviewing files that changed from the base of the PR and between 08eac48 and 9b020ff.

📒 Files selected for processing (131)
  • Devices/btt-panda-touch/source/module.cpp
  • Devices/cyd-2432s024c/source/module.cpp
  • Devices/cyd-2432s024r/source/module.cpp
  • Devices/cyd-2432s028r/source/module.cpp
  • Devices/cyd-2432s028rv3/source/module.cpp
  • Devices/cyd-2432s032c/source/module.cpp
  • Devices/cyd-3248s035c/source/module.cpp
  • Devices/cyd-4848s040c/source/module.cpp
  • Devices/cyd-8048s043c/source/module.cpp
  • Devices/cyd-e32r28t/source/module.cpp
  • Devices/cyd-e32r32p/source/module.cpp
  • Devices/elecrow-crowpanel-advance-28/source/module.cpp
  • Devices/elecrow-crowpanel-advance-35/source/module.cpp
  • Devices/elecrow-crowpanel-advance-50/source/module.cpp
  • Devices/elecrow-crowpanel-basic-28/source/module.cpp
  • Devices/elecrow-crowpanel-basic-35/source/module.cpp
  • Devices/elecrow-crowpanel-basic-50/source/module.cpp
  • Devices/es3c28p/source/module.cpp
  • Devices/generic-esp32/source/module.cpp
  • Devices/generic-esp32c6/source/module.cpp
  • Devices/generic-esp32p4/source/module.cpp
  • Devices/generic-esp32s3/source/module.cpp
  • Devices/guition-jc1060p470ciwy/source/module.cpp
  • Devices/guition-jc2432w328c/source/module.cpp
  • Devices/guition-jc8048w550c/source/module.cpp
  • Devices/heltec-wifi-lora-32-v3/source/module.cpp
  • Devices/lilygo-tdeck-max/source/module.cpp
  • Devices/lilygo-tdeck-plus/device.properties
  • Devices/lilygo-tdeck-plus/source/module.cpp
  • Devices/lilygo-tdeck/device.properties
  • Devices/lilygo-tdeck/source/module.cpp
  • Devices/lilygo-tdisplay-s3/source/module.cpp
  • Devices/lilygo-tdisplay/source/module.cpp
  • Devices/lilygo-tdongle-s3/source/module.cpp
  • Devices/lilygo-thmi/source/module.cpp
  • Devices/lilygo-tlora-pager/device.properties
  • Devices/lilygo-tlora-pager/source/module.cpp
  • Devices/m5stack-cardputer-adv/source/module.cpp
  • Devices/m5stack-cardputer/source/module.cpp
  • Devices/m5stack-core2/source/module.cpp
  • Devices/m5stack-cores3/source/module.cpp
  • Devices/m5stack-papers3/source/module.cpp
  • Devices/m5stack-stackchan/source/module.cpp
  • Devices/m5stack-stickc-plus2/source/module.cpp
  • Devices/m5stack-sticks3/source/module.cpp
  • Devices/m5stack-tab5/Source/module.cpp
  • Devices/unphone/source/module.cpp
  • Devices/waveshare-esp32-s3-geek/source/module.cpp
  • Devices/waveshare-s3-lcd-13/source/module.cpp
  • Devices/waveshare-s3-touch-lcd-128/source/module.cpp
  • Devices/waveshare-s3-touch-lcd-147/source/module.cpp
  • Devices/waveshare-s3-touch-lcd-43/source/module.cpp
  • Devices/wireless-tag-wt32-sc01-plus/source/module.cpp
  • Documentation/ideas.md
  • Drivers/EspLcdCompat/CMakeLists.txt
  • Drivers/EspLcdCompat/README.md
  • Drivers/EspLcdCompat/Source/EspLcdDisplay.cpp
  • Drivers/EspLcdCompat/Source/EspLcdDisplay.h
  • Drivers/EspLcdCompat/Source/EspLcdDisplayDriver.h
  • Drivers/EspLcdCompat/Source/EspLcdDisplayV2.cpp
  • Drivers/EspLcdCompat/Source/EspLcdDisplayV2.h
  • Drivers/EspLcdCompat/Source/EspLcdSpiDisplay.cpp
  • Drivers/EspLcdCompat/Source/EspLcdSpiDisplay.h
  • Drivers/EspLcdCompat/Source/EspLcdTouch.cpp
  • Drivers/EspLcdCompat/Source/EspLcdTouch.h
  • Drivers/EspLcdCompat/Source/EspLcdTouchDriver.cpp
  • Drivers/EspLcdCompat/Source/EspLcdTouchDriver.h
  • Drivers/audio-codec-module/README.md
  • Drivers/audio-stream-module/source/module.cpp
  • Drivers/aw88298-module/source/module.cpp
  • Drivers/aw9523b-module/source/module.cpp
  • Drivers/axp192-module/source/module.cpp
  • Drivers/axp2101-module/source/module.cpp
  • Drivers/axs5106-module/source/module.cpp
  • Drivers/bm8563-module/source/module.cpp
  • Drivers/bmi270-module/source/module.cpp
  • Drivers/bq24295-module/source/module.cpp
  • Drivers/bq25896-module/source/module.cpp
  • Drivers/bq27220-module/source/module.cpp
  • Drivers/button-control-module/source/module.cpp
  • Drivers/ch422g-module/source/module.cpp
  • Drivers/cst328-module/source/module.cpp
  • Drivers/cst66xx-module/source/module.cpp
  • Drivers/cst816s-module/source/module.cpp
  • Drivers/cst816t-module/source/module.cpp
  • Drivers/drv2605-module/source/module.cpp
  • Drivers/dummy-i2s-amp-module/source/module.cpp
  • Drivers/es7210-module/source/module.cpp
  • Drivers/es8311-module/source/module.cpp
  • Drivers/es8388-module/source/module.cpp
  • Drivers/ft5x06-module/source/module.cpp
  • Drivers/ft6x36-module/source/module.cpp
  • Drivers/gc9a01-module/source/module.cpp
  • Drivers/gt911-module/source/module.cpp
  • Drivers/hx8357-module/source/module.cpp
  • Drivers/ili9341-module/source/module.cpp
  • Drivers/ili9488-module/source/module.cpp
  • Drivers/ili9881c-module/source/module.cpp
  • Drivers/ina226-module/source/module.cpp
  • Drivers/jd9165-module/source/module.cpp
  • Drivers/jd9853-module/source/module.cpp
  • Drivers/lilygo-module/source/module.cpp
  • Drivers/m5pm1-module/source/module.cpp
  • Drivers/m5stack-module/source/module.cpp
  • Drivers/mpu6886-module/source/module.cpp
  • Drivers/pdm-mic-module/source/module.cpp
  • Drivers/pi4ioe5v6408-module/source/module.cpp
  • Drivers/py32ioexpander-module/source/module.cpp
  • Drivers/qmi8658-module/source/module.cpp
  • Drivers/rgb-display-module/source/module.cpp
  • Drivers/rx8130ce-module/source/module.cpp
  • Drivers/sc2356-module/source/module.cpp
  • Drivers/ssd1306-module/source/module.cpp
  • Drivers/st7123-module/source/module.cpp
  • Drivers/st7701-module/source/module.cpp
  • Drivers/st7735-module/source/module.cpp
  • Drivers/st7789-i8080-module/source/module.cpp
  • Drivers/st7789-module/source/module.cpp
  • Drivers/st7796-i8080-module/source/module.cpp
  • Drivers/st7796-module/source/module.cpp
  • Drivers/sy6970-module/source/module.cpp
  • Drivers/tca8418-module/source/module.cpp
  • Drivers/tca9534-module/source/module.cpp
  • Drivers/tca95xx-16bit-module/source/module.cpp
  • Drivers/xl9555-module/source/module.cpp
  • Drivers/xpt2046-module/source/module.cpp
  • Drivers/xpt2046-softspi-module/source/module.cpp
  • Modules/crypt-module/source/module.cpp
  • Modules/hal-device-module/source/module.cpp
  • TactilityKernel/include/tactility/module.h
  • TactilityKernel/source/module.cpp
💤 Files with no reviewable changes (15)
  • Devices/lilygo-tdeck-plus/source/module.cpp
  • Drivers/EspLcdCompat/Source/EspLcdDisplay.h
  • Drivers/EspLcdCompat/CMakeLists.txt
  • Drivers/EspLcdCompat/Source/EspLcdDisplayV2.cpp
  • Drivers/EspLcdCompat/Source/EspLcdTouch.h
  • Drivers/EspLcdCompat/Source/EspLcdTouchDriver.h
  • Drivers/EspLcdCompat/Source/EspLcdDisplayDriver.h
  • Drivers/EspLcdCompat/README.md
  • Drivers/EspLcdCompat/Source/EspLcdSpiDisplay.h
  • Devices/cyd-2432s024c/source/module.cpp
  • Drivers/EspLcdCompat/Source/EspLcdSpiDisplay.cpp
  • Drivers/EspLcdCompat/Source/EspLcdDisplayV2.h
  • Drivers/EspLcdCompat/Source/EspLcdDisplay.cpp
  • Drivers/EspLcdCompat/Source/EspLcdTouchDriver.cpp
  • Drivers/EspLcdCompat/Source/EspLcdTouch.cpp

Comment thread Drivers/audio-codec-module/README.md
Comment thread Drivers/cst66xx-module/source/module.cpp Outdated
Comment thread Drivers/mpu6886-module/source/module.cpp Outdated
Comment thread TactilityKernel/source/module.cpp
Comment on lines +62 to 80
if (module->start != nullptr) {
auto error = module->start();
if (error != ERROR_NONE) {
return error;
}
}

if (module->drivers != nullptr) {
auto* driver_location = module->drivers;
while (*driver_location != nullptr) {
auto driver = *driver_location;
driver_construct_add(driver);
driver_location++;
}
}

internal->started = true;
return ERROR_NONE;
}

@coderabbitai coderabbitai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm the exact signatures of driver_construct_add / driver_remove_destruct
fd -e h -p 'driver.h' TactilityKernel
rg -n 'driver_construct_add|driver_remove_destruct' TactilityKernel/include/tactility/driver.h -A3 -B3

Repository: TactilityProject/Tactility

Length of output: 687

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== module.h relevant lines =="
rg -n 'struct Module|drivers|struct Driver' TactilityKernel/include/tactility/module.h -A40 -B20

echo "== module.cpp relevant lines =="
sed -n '1,140p' TactilityKernel/source/module.cpp | cat -n

echo "== const Driver ** declarations/usages =="
rg -n 'const struct Driver\\*\\* drivers|Driver\\*\\*|drivers\\]' TactilityKernel -g '*.h' -g '*.c' -g '*.cpp' -g '*.hpp'

Repository: TactilityProject/Tactility

Length of output: 15848

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== C++ const conversion compile probe =="
cat > /tmp/const_driver_probe.cpp <<'CPP'
struct Driver { int x = 0; };
extern "C" {
  using error_t = int;
  enum { ERROR_NONE = 0 };
  error_t driver_construct_add(struct Driver* driver) { driver->x++; return ERROR_NONE; }
  error_t driver_remove_destruct(struct Driver* driver) { driver->x--; return ERROR_NONE; }
  error_t module_start() {
    const struct Driver* driver_list[] = { nullptr };
    for (const struct Driver** it = driver_list; *it != nullptr; it++) {
      const struct Driver* driver = *it;
      driver_construct_add(driver);
      driver_remove_destruct(driver);
    }
    return ERROR_NONE;
  }
}
int main() { return module_start(); }
CPP
if command -v c++ >/dev/null 2>&1; then
  c++ -std=c++17 -Werror /tmp/const_driver_probe.cpp -o /tmp/const_driver_probe 2>&1
else
  echo "c++ not available"
fi

echo "== module.h field type text =="
sed -n '35,65p' TactilityKernel/include/tactility/module.h | cat -n

Repository: TactilityProject/Tactility

Length of output: 1535

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== driver.cpp lifecycle implementations =="
sed -n '31,96p' TactilityKernel/source/driver.cpp | cat -n

echo "== driver.h lifecycle signatures =="
sed -n '40,96p' TactilityKernel/include/tactility/driver.h | cat -n

echo "== all const_driver declarations/usages =="
rg -n 'const struct Driver' TactilityKernel -g '*.h' -g '*.c' -g '*.cpp' -g '*.hpp'

Repository: TactilityProject/Tactility

Length of output: 4437

Propagate failures from the module driver lifecycle loops.

module_start() and module_stop() now discard the return codes from driver_construct_add() / driver_remove_destruct(), so construction/removal errors are ignored. In module_start(), the module is marked started and returns ERROR_NONE even if a driver failed; in module_stop(), module->stop() is called after driver removal and an early return can leave internal->started true. Capture these return values and return them so module start/stop status is not silently corrupted.

Also applies to: 94-101

Comment thread TactilityKernel/source/module.cpp
Comment on lines +87 to 113
error_t module_stop(Module* module) {
LOG_I(TAG, "stop %s", module->name);

auto* internal = module->internal;
if (internal == nullptr) return ERROR_INVALID_STATE;
if (!internal->started) return ERROR_NONE;

error_t error = module->stop();
if (error != ERROR_NONE) {
return error;
if (module->drivers != nullptr) {
auto* driver_location = module->drivers;
while (*driver_location != nullptr) {
auto driver = *driver_location;
driver_remove_destruct(driver);
driver_location++;
}
}

if (module->stop != nullptr) {
auto error = module->stop();
if (error != ERROR_NONE) {
return error;
}

}

internal->started = false;
return error;
return ERROR_NONE;
}

@coderabbitai coderabbitai Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Double-destruct risk if module->stop() fails after drivers were already removed.

Drivers are removed/destructed first (lines 94-101), then module->stop() is conditionally invoked. If module->stop() returns an error, the function returns at line 106 before internal->started = false runs (line 111). internal->started stays true, so a later module_stop retry will pass the if (!internal->started) guard again and re-run driver_remove_destruct on pointers that were already destructed.

🐛 Proposed fix: mark stopped once drivers are torn down, independent of the stop callback's outcome 
     if (module->drivers != nullptr) {
         auto* driver_location = module->drivers;
         while (*driver_location != nullptr) {
             auto driver = *driver_location;
             driver_remove_destruct(driver);
             driver_location++;
         }
     }

+    internal->started = false;
+
     if (module->stop != nullptr) {
         auto error = module->stop();
         if (error != ERROR_NONE) {
             return error;
         }
-
     }

-    internal->started = false;
     return ERROR_NONE;
 }
📝 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.

Suggested change
error_t module_stop(Module* module) {
LOG_I(TAG, "stop %s", module->name);
auto* internal = module->internal;
if (internal == nullptr) return ERROR_INVALID_STATE;
if (!internal->started) return ERROR_NONE;
error_t error = module->stop();
if (error != ERROR_NONE) {
return error;
if (module->drivers != nullptr) {
auto* driver_location = module->drivers;
while (*driver_location != nullptr) {
auto driver = *driver_location;
driver_remove_destruct(driver);
driver_location++;
}
}
if (module->stop != nullptr) {
auto error = module->stop();
if (error != ERROR_NONE) {
return error;
}
}
internal->started = false;
return error;
return ERROR_NONE;
}
error_t module_stop(Module* module) {
LOG_I(TAG, "stop %s", module->name);
auto* internal = module->internal;
if (internal == nullptr) return ERROR_INVALID_STATE;
if (!internal->started) return ERROR_NONE;
if (module->drivers != nullptr) {
auto* driver_location = module->drivers;
while (*driver_location != nullptr) {
auto driver = *driver_location;
driver_remove_destruct(driver);
driver_location++;
}
}
internal->started = false;
if (module->stop != nullptr) {
auto error = module->stop();
if (error != ERROR_NONE) {
return error;
}
}
return ERROR_NONE;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@KenVanHoeylandt