Create PWM driver initial version by RuiqiLiu2014 · Pull Request #12 · UWCubeSat/HS2-CDH

RuiqiLiu2014 · 2026-05-15T04:40:14Z

Description

Our first draft of the PWM driver, does not have unit tests.

Artifacts for PR #12 (DO NOT CHANGE)

Copilot

Pull request overview

Adds an initial F Prime Linux sysfs PWM driver component, defining its ports, status enum, events, C++ implementation, and local CMake target.

Changes:

Defines the Drv.LinuxPwmDriver passive component and PWM control ports.
Implements sysfs-based period, duty cycle, enable, and export handling.
Adds a component-level CMake library target, with unit tests left disabled.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.

File	Description
`FlightComputer/Components/PwmDriver/LinuxPwmDriver.hpp`	Declares the PWM driver component implementation and internal state.
`FlightComputer/Components/PwmDriver/LinuxPwmDriver.cpp`	Implements sysfs PWM export and control handlers.
`FlightComputer/Components/PwmDriver/Driver.fpp`	Defines the FPP component, ports, events, interface, and status enum.
`FlightComputer/Components/PwmDriver/CMakeLists.txt`	Registers the PWM driver library and includes a commented-out unit test target.

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

+register_fprime_library(
+    AUTOCODER_INPUTS
+        "${CMAKE_CURRENT_LIST_DIR}/Driver.fpp"
+    SOURCES
+        "${CMAKE_CURRENT_LIST_DIR}/LinuxPwmDriver.cpp"


+#ifndef Drv_LinuxPwmDriver_HPP
+#define Drv_LinuxPwmDriver_HPP
+
+#include "LinuxPwmDrv/Components/Driver/LinuxPwmDriverComponentAc.hpp"


+// \brief  cpp file for LinuxPwmDriver component implementation class
+// ======================================================================
+
+#include "LinuxPwmDrv/Components/Driver/LinuxPwmDriver.hpp"


+# register_fprime_ut(
+#     AUTOCODER_INPUTS
+#         "${CMAKE_CURRENT_LIST_DIR}/Driver.fpp"
+#     SOURCES
+#         "${CMAKE_CURRENT_LIST_DIR}/test/ut/DriverTestMain.cpp"
+#         "${CMAKE_CURRENT_LIST_DIR}/test/ut/DriverTester.cpp"
+#     DEPENDS
+#         STest # For rules-based testing
+#     UT_AUTO_HELPERS
+# )


+    int fd = ::open(exportPath, O_WRONLY);
+    if (fd < 0) {
+        // Log the error event using errno to know exactly why it failed (e.g., EACCES)
+        this->log_WARNING_HI_OpenChannelError(chipNum, channelNum, errno);
+        return false;


+    // 2. Save the base path for our handlers to use later
+    snprintf(this->m_basePath, sizeof(this->m_basePath), "/sys/class/pwm/pwmchip%u/pwm%u", chipNum, channelNum);
+
+    // 3. Mark as opened and log success
+    this->m_opened = true;


+    if (bytesWritten != len) {
+        // The file opened, but the write failed (e.g., channel already exported or invalid)
+        this->log_WARNING_HI_OpenChannelError(chipNum, channelNum, errno);
+        return false;
+    }
+
+    // 2. Save the base path for our handlers to use later
+    snprintf(this->m_basePath, sizeof(this->m_basePath), "/sys/class/pwm/pwmchip%u/pwm%u", chipNum, channelNum);
+


+// ----------------------------------------------------------------------
+// Component construction and destruction
+// ----------------------------------------------------------------------
+
+LinuxPwmDriver ::LinuxPwmDriver(const char* const compName) : LinuxPwmDriverComponentBase(compName) {}
+
+LinuxPwmDriver ::~LinuxPwmDriver() {}


+    // Write the string and immediately close the file
+    ssize_t bytesWritten = ::write(fd, channelStr, len);
+    ::close(fd);
+
+    if (bytesWritten != len) {
+        // The file opened, but the write failed (e.g., channel already exported or invalid)
+        this->log_WARNING_HI_OpenChannelError(chipNum, channelNum, errno);


+    @ A passive driver for Linux sysfs PWM
+    passive component LinuxPwmDriver {


h-badams · 2026-05-25T21:50:25Z

if you could fix the failing CI checks and review the Copilot suggestions (in particular, making sure that the OS-level driver opens and closes correctly), this would be great.

Adding @alexisgmm as a reviewer since they are writing the ADCS software that will actually use this driver.

808sAndBlues · 2026-05-31T21:37:08Z

+Drv::PwmStatus LinuxPwmDriver::open(U32 chipNum, U32 channelNum) {
+    // 1. Export the channel (Tell Linux we want to use it)
+    char exportPath[256];
+    snprintf(exportPath, sizeof(exportPath), "/sys/class/pwm/pwmchip%u/export", chipNum);


Return value should be checked, system calls always have a chance of failing.

808sAndBlues · 2026-05-31T21:38:01Z

+
+Drv::PwmStatus LinuxPwmDriver::open(U32 chipNum, U32 channelNum) {
+    // 1. Export the channel (Tell Linux we want to use it)
+    char exportPath[256];


Magic number, define size as a #define or constexpr U8 somewhere in the header

808sAndBlues · 2026-05-31T21:38:13Z

+    }
+
+    // Convert the channel number to a string to write to the file
+    char channelStr[32];


Ditto from above

808sAndBlues · 2026-05-31T21:38:51Z

+    int len = snprintf(channelStr, sizeof(channelStr), "%u", channelNum);
+
+    // Write the string and immediately close the file
+    ssize_t bytesWritten = ::write(fd, channelStr, len);
+    ::close(fd);


Return values should be checked and logged for errors

808sAndBlues · 2026-05-31T21:39:22Z

+    }
+
+    // 2. Save the base path for our handlers to use later
+    snprintf(this->m_basePath, sizeof(this->m_basePath), "/sys/class/pwm/pwmchip%u/pwm%u", chipNum, channelNum);


Check return code please

808sAndBlues · 2026-05-31T21:39:45Z

+}
+
+Drv::PwmStatus LinuxPwmDriver::writeSysfsInt(const char* file, U32 value) {
+    char fullPath[512];


Define size as constant please

808sAndBlues · 2026-05-31T21:39:53Z

+
+Drv::PwmStatus LinuxPwmDriver::writeSysfsInt(const char* file, U32 value) {
+    char fullPath[512];
+    snprintf(fullPath, sizeof(fullPath), "%s/%s", this->m_basePath, file);


Check return code

808sAndBlues · 2026-05-31T21:40:32Z

+    int len = snprintf(valStr, sizeof(valStr), "%u", value);
+
+    // Write the buffer directly
+    ssize_t bytesWritten = ::write(fd, valStr, len);
+
+    ::close(fd);


These lines seem to be repeated, any chance we can have these put into a function somehow?

808sAndBlues · 2026-05-31T21:42:23Z

+    U32 m_periodNs = 0;
+
+    // Buffer to hold the base path (e.g., "/sys/class/pwm/pwmchip0/pwm1")
+    char m_basePath[256];


Ditto from previous comments, change size to constant defined somewhere in this header

808sAndBlues · 2026-05-31T21:43:31Z

+    // Handler implementations for typed input ports
+    // ----------------------------------------------------------------------
+
+    //! Handler implementation for pwmEnable


@MahirEmran @h-badams @alexisgmm I see that F prime is writing comments inline with the parameters, should we change these to be doxygen (as in, above the function)? My vote is yes considering our pipelines ask for this formatting.

This reverts commit f13c322.

Create PWM driver initial version

e238f1a

h-badams requested a review from Copilot May 15, 2026 05:31

Copilot started reviewing on behalf of h-badams May 15, 2026 05:32 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

h-badams requested a review from alexisgmm May 25, 2026 21:48

808sAndBlues reviewed May 31, 2026

View reviewed changes

address comments in pull request

921aedd

RuiqiLiu2014 force-pushed the pwm-driver branch from 1b36d97 to 921aedd Compare June 1, 2026 00:42

hyuan8 and others added 6 commits June 1, 2026 16:10

Update include path for LinuxPwmDriverComponentAc

b2b15dc

Enable unit tests for PwmDriver in CMakeLists.txt

bb31841

added configuration method

67910a6

added tests, need to review them

f13c322

Revert "added tests, need to review them"

d6013e6

This reverts commit f13c322.

generate unit tests

308e021

Navigation Menu

Uh oh!

Create PWM driver initial version - #12

Create PWM driver initial version#12
RuiqiLiu2014 wants to merge 8 commits into
mainfrom
pwm-driver

RuiqiLiu2014 commented May 15, 2026 •

edited by github-actions Bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

h-badams commented May 25, 2026 •

edited

Loading

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

808sAndBlues May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@ A passive driver for Linux sysfs PWM
		passive component LinuxPwmDriver {

Uh oh!

Conversation

RuiqiLiu2014 commented May 15, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Artifacts for PR #12 (DO NOT CHANGE)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

h-badams commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RuiqiLiu2014 commented May 15, 2026 •

edited by github-actions Bot

Loading

h-badams commented May 25, 2026 •

edited

Loading