Skip to content

Conversation

@andre-stefanov
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings November 27, 2025 15:29
Copilot finished reviewing on behalf of andre-stefanov November 27, 2025 15:33
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a comprehensive refactoring of the stepper motor overlay configuration and thread management infrastructure. The main purpose is to modernize the device configuration approach by migrating from Zephyr aliases to chosen nodes, introduce a base Thread class to reduce code duplication, and improve the stepper driver API usage. The changes also establish proper test infrastructure with fake stepper drivers, separating test concerns from the main application.

Key Changes

  • Migrated device configuration from devicetree aliases and zephyr,user properties to chosen nodes (focuser,uart, focuser,stepper, focuser,stepper-drv)
  • Introduced a base Thread class to eliminate duplication in thread creation logic between FocuserThread and UartThread
  • Created dedicated test infrastructure using Zephyr's fake stepper drivers, removing qemu_cortex_m0 support from the main application
  • Updated stepper driver API calls from stepper_enable/stepper_disable to stepper_drv_enable/stepper_drv_disable

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/app/focuser/testcase.yaml Adds test configuration targeting qemu_cortex_m0 with focuser tag
tests/app/focuser/src/main.cpp Implements comprehensive test suite for Focuser initialization, speed control, and stop functionality using fake stepper drivers
tests/app/focuser/prj.conf Configures test environment with ztest, C++20, and fake stepper driver support
tests/app/focuser/boards/qemu_cortex_m0.overlay Defines devicetree overlay with fake stepper controller and driver for testing
tests/app/focuser/boards/qemu_cortex_m0.conf Test-specific board configuration (with outdated comment about H-bridge driver)
tests/app/focuser/CMakeLists.txt Test build configuration linking to application sources
app/src/main.cpp Refactors to use new configuration namespace structure and removes console device check
app/src/ZephyrStepper.cpp Updates to use new stepper driver API (stepper_drv_enable/disable)
app/src/UartThread.hpp Converts to inherit from Thread base class
app/src/UartThread.cpp Refactors to use Thread base class constructor and start_thread() method
app/src/Thread.hpp Introduces new base class for thread management with protected start_thread() method
app/src/Thread.cpp Implements base thread creation logic with validation and naming support
app/src/FocuserThread.hpp Converts to inherit from Thread base class
app/src/FocuserThread.cpp Refactors to use Thread base class constructor and start_thread() method
app/src/Configuration.hpp Major refactor: uses chosen nodes instead of aliases, consolidates thread configuration into nested namespaces
app/sample.yaml Removes qemu_cortex_m0 from integration platforms (now only in tests)
app/prj.conf Reorganizes configuration comments and adds CONFIG_COUNTER support
app/boards/qemu_cortex_m0.overlay Removes qemu_cortex_m0 overlay from app (moved to test infrastructure)
app/boards/esp32s3_devkitc_procpu.overlay Updates to use chosen nodes and separates stepper controller from driver, with incomplete comment documentation
app/CMakeLists.txt Adds Thread.cpp to build sources
.github/workflows/docs.yml Expands workflow trigger syntax from compact to explicit format
Comments suppressed due to low confidence (1)

tests/app/focuser/boards/qemu_cortex_m0.conf:5

  • The comment is outdated. It mentions "swap in Zephyr's GPIO H-bridge stepper driver", but the test now uses the fake stepper driver (zephyr,fake-stepper-controller and zephyr,fake-stepper-driver) as configured in the overlay file, not the H-bridge driver. The comment should be updated to reflect the use of the fake stepper driver for testing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


/**
* Moonlite focuser control UART configuration.
* Used for
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The comment is incomplete: "Used for" appears to be cut off mid-sentence. Please complete the documentation to explain what uart1 is used for (appears to be for logging and console output based on the chosen nodes).

Suggested change
* Used for
* Used for logging and console output.

Copilot uses AI. Check for mistakes.
Comment on lines 21 to 23
zephyr,user {
uart_handler = <&uart0>;
};
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The zephyr,user node with uart_handler property appears to be unused in the test code. The test directly accesses the stepper devices via DT_ALIAS(stepper) and DT_ALIAS(stepper_drv) without using any UART configuration. Consider removing this unused node to keep the test configuration minimal and clear.

Suggested change
zephyr,user {
uart_handler = <&uart0>;
};

Copilot uses AI. Check for mistakes.
Comment on lines 7 to 11
* (PROCPU). The zephyr,gpio-step-dir-controller drives the step/dir pins
* referenced by the "stepper" alias, while the dedicated TMC2209 device is
* referenced by the "stepper-drv" alias so the application can toggle the
* external driver rails. The zephyr,user node supplies the UART phandle
* consumed by the application.
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The comment references outdated concepts:

  • Mentions "stepper" and "stepper-drv" aliases, but the code now uses chosen nodes instead
  • Mentions "zephyr,user node supplies the UART phandle", but zephyr,user is no longer used

The comment should be updated to reflect that the chosen nodes (focuser,uart, focuser,stepper, focuser,stepper-drv) are used to configure the devices.

Suggested change
* (PROCPU). The zephyr,gpio-step-dir-controller drives the step/dir pins
* referenced by the "stepper" alias, while the dedicated TMC2209 device is
* referenced by the "stepper-drv" alias so the application can toggle the
* external driver rails. The zephyr,user node supplies the UART phandle
* consumed by the application.
* (PROCPU). Device configuration is performed using chosen nodes:
* - focuser,uart: selects the UART device for the focuser console
* - focuser,stepper: selects the step/dir controller for the stepper motor
* - focuser,stepper-drv: selects the TMC2209 stepper driver device
* These chosen nodes are used by the application to access the appropriate hardware.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants