LED: Fix artefacts.

[lundril]

Basic idea to change the state of the Pins:

1. Tri-State all (will TURN OFF all LEDs)
2. Define correct High/Low value for Pins
3. Drive pins which need driving (will TURN ON correct LEDs)

Summary by Sourcery

Bug Fixes:

• Fixed LED display artefacts.

[sourcery-ai]

Reviewer's Guide by Sourcery

This pull request fixes LED artefacts by changing how the GPIO pins are controlled. It first sets all pins to a tri-state (off) mode, then sets the desired high/low values for each pin, and finally drives only the pins that need to be turned on. This ensures that only the correct LEDs are illuminated and prevents unintended LED behavior.

Sequence diagram for GPIO pin control flow

sequenceDiagram
    participant GPIO as GPIO Controller
    participant DIR as Direction Register
    participant OUT as Output Register
    participant DRV as Drive Strength Register

    GPIO->>DIR: Clear all pin directions (tri-state)
    GPIO->>OUT: Set output values
    alt Drive strength is 20mA
        GPIO->>DRV: Update drive strength
    end
    GPIO->>DIR: Enable selected pin directions

Loading

Class diagram for LED driver structure

classDiagram
    class pinctrl_t {
        +uint8_t* cfg_buf
        +uint8_t* port_buf
        +uint32_t pin
    }

    class GPIO_Constants {
        +GPIO_PD_DRV
        +GPIO_DIR
        +GPIO_OUT
    }

    class LED_Driver {
        -pinctrl_t led_pins[]
        -uint32_t drive_strength
        +led_setDriveStrength(is_20mA)
        -gpio_buf_apply()
        -gpio_buf_set()
    }

    LED_Driver --> pinctrl_t
    LED_Driver --> GPIO_Constants

Loading

State diagram for LED pin control sequence

stateDiagram-v2
    [*] --> AllPinsTriState: 1. Turn off all LEDs
    AllPinsTriState --> SetPinValues: 2. Set High/Low values
    SetPinValues --> DrivePins: 3. Drive selected pins
    DrivePins --> [*]: LEDs in correct state

    note right of AllPinsTriState: All pins floating
    note right of SetPinValues: Configure output values
    note right of DrivePins: Enable only needed pins

Loading

File-Level Changes

Change	Details	Files
Implement a three-phase pin control process to eliminate LED artefacts.	Tri-state all pins to turn off all LEDs initially. Set the desired high/low values for each pin. Drive only the pins that need to be turned on, ensuring only the correct LEDs are illuminated.	src/leddrv.c
Refactor GPIO pin control logic for clarity and efficiency.	Remove unnecessary conditional checks. Simplify pin manipulation logic. Improve code readability by adding comments and whitespace.	src/leddrv.c

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time. You can also use
  this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey @lundril - I've reviewed your changes and they look great!

Here's what I looked at during the review

• 🟢 General issues: all looks good
• 🟡 Security: 1 issue found
• 🟢 Testing: all looks good
• 🟡 Complexity: 1 issue found
• 🟢 Documentation: all looks good

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

🚨 suggestion (security): Consider protecting the break-before-make sequence from interrupts

While the break-before-make sequence prevents shoot-through current, interrupts between the two dir register updates could cause temporary pin state issues. Consider using critical sections if this is a concern in your system.

Suggested implementation:

#include <stdint.h>
#include <zephyr/irq.h>

	unsigned int key = irq_lock();
	dir_val = (dir_val & ~*mask); // first turn off drive to all Pins
	*dir = dir_val;
	*out = out_val;
	if (drive_strength) {
		*drv = drv_val;
	}
	dir_val = dir_val | (*cfg & *mask); // now turn on drive to right Pins
	*dir = dir_val;
	irq_unlock(key);

Note: This assumes the code is running on Zephyr RTOS. If using a different RTOS or bare metal, you'll need to use the appropriate critical section / interrupt disable functions for your system.

[sourcery-ai]

issue (complexity): Consider restructuring gpio_buf_apply to clarify the hardware-driven timing sequence by grouping register access, using const for safety, and adding explanatory comments.

The gpio_buf_apply function's complexity appears driven by hardware timing requirements, but needs better documentation and structure. Consider:

static void gpio_buf_apply(
        volatile uint8_t *gpio_base,
        uint32_t *port, uint32_t *cfg,
        uint32_t *mask)
{
    // Define register pointers
    volatile uint32_t * const drv = (volatile uint32_t *)(gpio_base + GPIO_PD_DRV);
    volatile uint32_t * const dir = (volatile uint32_t *)(gpio_base + GPIO_DIR);
    volatile uint32_t * const out = (volatile uint32_t *)(gpio_base + GPIO_OUT);

    // Read current register values
    const uint32_t drv_val = drive_strength ? *drv : 0;
    const uint32_t dir_val = *dir;
    const uint32_t out_val = *out;

    // Prepare new values
    const uint32_t new_drv = (drv_val & ~*mask) | (*cfg & *mask);
    const uint32_t new_out = (out_val & ~*mask) | (*port & *mask);

    // Hardware timing sequence:
    // 1. Disable all affected pins
    *dir = dir_val & ~*mask;

    // 2. Update output and drive strength while pins are disabled
    *out = new_out;
    if (drive_strength) {
        *drv = new_drv;
    }

    // 3. Enable affected pins with new configuration
    *dir = (dir_val & ~*mask) | (*cfg & *mask);
}

This restructuring:

1. Groups register access into clear phases
2. Uses const to prevent accidental modifications
3. Adds comments explaining the timing sequence
4. Makes the hardware requirements more obvious through code structure

[kienvo]

Wow. Now I see what caused these artefacts. It was a real headache for me.
Your reimplementation seems to resolve #48.
Pinging @fcartegnie =]]