Add display brightness control #83

weberval · 2025-06-21T15:52:01Z

Summary by Sourcery

Introduce brightness control by adding a Brightness enum and integrating it into the protocol header, payload buffer, and CLI configuration, along with related tests and minor documentation fixes.

New Features:

Add Brightness enum with four levels and conversions to/from u8
Extend protocol header and payload buffer to include brightness control
Enable CLI config and payload generation to set display brightness

Enhancements:

Adjust MAGIC constant length from 6 to 5 to accommodate brightness field
Add set_brightness method and default initialization for PayloadBuffer

Documentation:

Fix typos in documentation comments (e.g., "around", "through", "consists", buffer instantiation)

Tests:

Add unit test for brightness to u8 conversion

sourcery-ai

Hey @weberval - I've reviewed your changes - here's some feedback:

The brightness_to_u8 test calls Brightness::from(i.0) incorrectly – it should directly use u8::from(i.0) since you only implemented From<Brightness> for u8.
Consider providing a builder-style .brightness() setter or allowing brightness to be modified on an existing PayloadBuffer for consistency with other style setters instead of only via the constructor.
Changing the header size by removing a magic byte and inserting brightness breaks protocol compatibility; ensure you document this change clearly and bump the protocol version accordingly.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- The `brightness_to_u8` test calls `Brightness::from(i.0)` incorrectly – it should directly use `u8::from(i.0)` since you only implemented `From<Brightness> for u8`.
- Consider providing a builder-style `.brightness()` setter or allowing brightness to be modified on an existing `PayloadBuffer` for consistency with other style setters instead of only via the constructor.
- Changing the header size by removing a magic byte and inserting `brightness` breaks protocol compatibility; ensure you document this change clearly and bump the protocol version accordingly.

## Individual Comments

### Comment 1
<location> `src/protocol.rs:536` </location>
<code_context>
+            (Brightness::OneQuarter, 0x30),
+        ];
+
+        for i in VALID_BRIGHTNESS_VALUES {
+            assert_eq!(u8::from(Brightness::from(i.0)), i.1);
+        }
</code_context>

<issue_to_address>
Redundant use of Brightness::from in test.

You can simplify `u8::from(Brightness::from(i.0))` to `u8::from(i.0)` since `i.0` is already a Brightness.
</issue_to_address>

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

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

src/protocol.rs

weberval · 2025-07-10T19:16:53Z

I'm still waiting for review. Is there any chance this happens in the next month?

mgjm · 2025-07-10T23:04:02Z

Before I start with the review, can you please point me to the place where those brightness values are used in the firmware.

I could only find a brightness value controlled by button presses or a new BLE characteristic. Or is this only implemented in the OEM firmware?

mgjm · 2025-07-15T11:43:32Z

@weberval I don't want to put you under pressure or anything, I just want to mention you explicitly in case you weren't notified about the previous message.

weberval · 2025-07-15T15:55:55Z

It seems it is not (yet) implemented in the OSS firmware.

Instead, it seems like the "magic" header is longer than required and the brightness value is the last byte of that header. (see firmware vs my protocol implementation)

I have not found any validation of the magic header wang\0\0 in the OSS firmware, so I think it should be fine already implementing this feature in the client library and opening an issue on the OSS firmware repository.

mgjm · 2025-07-15T16:14:00Z

@weberval Do you have a link to a fork of the firmware with the brightness already implemented? Is this implemented in any other software to talk to the badges (e.g. Android App, Go implementation, etc.)?

If this is not implemented anywhere else, how / why did you pick the values 0x00, 0x10, 0x20 and 0x30? How are the other bits defined (0x01 ..= 0x0F and 0x40 ..= 0xFF)? (Must be zero, reserved for future use, etc.)

weberval · 2025-07-15T16:22:02Z

I used the values from the python project. I don't know what padding is applied to the value in the proprietary firmware or what the other bits could be used for, but those values seem to work.

mgjm · 2025-07-15T17:22:42Z

Ok, awesome. So it is from a commit from 2019 (fossasia/led-name-badge-ls32@6640ee3). Therefore I would assume that this is implemented in the proprietary firmware from the manufacturer.

Then lets look at the actual code...

mgjm

Thanks for the PR. I have added a few comments. And thanks for fixing a lot of my typos 👍

Cargo.lock

Cargo.toml

LICENSE-MIT

src/main.rs

src/protocol.rs

src/main.rs

src/protocol.rs

mgjm · 2025-07-15T19:01:20Z

Note

I just updated the dependencies and fixed the new clippy lints. You can pull these changes into your PR.

weberval · 2025-07-16T18:11:00Z

@sourcery-ai dismiss

weberval · 2025-07-16T18:12:00Z

@sourcery-ai review

SourceryAI

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

Prompt for AI Agents

Please address the comments from this code review:
## Individual Comments

### Comment 1
<location> `src/protocol.rs:214` </location>
<code_context>
+    }
+}
+
+impl TryFrom<u8> for Brightness {
+    type Error = TryFromIntError;
+
+    fn try_from(value: u8) -> Result<Self, Self::Error> {
+        Ok(match value {
+            0x00 => Self::Full,
+            0x10 => Self::ThreeQuarters,
+            0x20 => Self::Half,
+            0x30 => Self::OneQuarter,
+            _ => return Err(u8::try_from(-1).unwrap_err()),
+        })
+    }
</code_context>

<issue_to_address>
Using u8::try_from(-1) to generate an error is indirect and may be confusing.

Use a more explicit error, like core::num::TryFromIntError::default() or a custom error type, to clarify intent and avoid depending on u8::try_from(-1).
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
+impl TryFrom<u8> for Brightness {
+    type Error = TryFromIntError;
+
+    fn try_from(value: u8) -> Result<Self, Self::Error> {
+        Ok(match value {
+            0x00 => Self::Full,
+            0x10 => Self::ThreeQuarters,
+            0x20 => Self::Half,
+            0x30 => Self::OneQuarter,
+            _ => return Err(u8::try_from(-1).unwrap_err()),
+        })
+    }
=======
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct BrightnessParseError;
+
+impl core::fmt::Display for BrightnessParseError {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "invalid value for Brightness")
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for BrightnessParseError {}
+
+impl TryFrom<u8> for Brightness {
+    type Error = BrightnessParseError;
+
+    fn try_from(value: u8) -> Result<Self, Self::Error> {
+        match value {
+            0x00 => Ok(Self::Full),
+            0x10 => Ok(Self::ThreeQuarters),
+            0x20 => Ok(Self::Half),
+            0x30 => Ok(Self::OneQuarter),
+            _ => Err(BrightnessParseError),
+        }
+    }
>>>>>>> REPLACE

</suggested_fix>

### Comment 2
<location> `src/protocol.rs:342` </location>
<code_context>
+    }
+
     /// Return the current number of messages
     pub fn num_messages(&mut self) -> usize {
         self.num_messages as usize
</code_context>

<issue_to_address>
The num_messages method takes &mut self but does not mutate state.

Changing the receiver to &self would improve flexibility since no mutation occurs.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
    /// Return the current number of messages
    pub fn num_messages(&mut self) -> usize {
        self.num_messages as usize
        &self.data
    }
=======
    /// Return the current number of messages
    pub fn num_messages(&self) -> usize {
        self.num_messages as usize
    }
>>>>>>> REPLACE

</suggested_fix>

Hi @weberval! 👋

Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀

Install the sourcery-ai bot to get automatic code reviews on every pull request ✨

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

src/protocol.rs

SourceryAI · 2025-07-16T18:12:39Z

src/protocol.rs

        &self.data
    }


suggestion: The num_messages method takes &mut self but does not mutate state.

Changing the receiver to &self would improve flexibility since no mutation occurs.

Suggested change

&self.data

}

/// Return the current number of messages

pub fn num_messages(&self) -> usize {

self.num_messages as usize

}

Not related to this change set, but should be fixed 😅

sourcery-ai

Hey @weberval - I've reviewed your changes - here's some feedback:

The TryFrom implementation for Brightness uses a workaround (u8::try_from(-1).unwrap_err()) to generate errors—consider defining a dedicated error type or using a more idiomatic approach for invalid values.
Changing the protocol header (MAGIC length changed and a brightness byte added) is a breaking change—make sure the version bump follows semver (major version) or include upgrade notes.
Consider adding unit tests for invalid brightness inputs to verify that TryFrom correctly returns Err for out-of-range values.

Prompt for AI Agents

Please address the comments from this code review:
## Overall Comments
- The TryFrom<u8> implementation for Brightness uses a workaround (`u8::try_from(-1).unwrap_err()`) to generate errors—consider defining a dedicated error type or using a more idiomatic approach for invalid values.
- Changing the protocol header (MAGIC length changed and a brightness byte added) is a breaking change—make sure the version bump follows semver (major version) or include upgrade notes.
- Consider adding unit tests for invalid brightness inputs to verify that TryFrom<u8> correctly returns Err for out-of-range values.

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

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

mgjm

Just a few tiny points left. But I thought a bit about the current representation in the config file:

brightness = "full"
brightness = "three-quarters"
brightness = "half"
brightness = "one-quarter"

Feels a bit weird to write the names out. Alternative representations:

Percentage numbers like in the python implementation:

brightness = 100
brightness = 75
brightness = 50
brightness = 25

Percentages but with a percent sign (needs to be a string):

brightness = "100%"
brightness = "75%"
brightness = "50%"
brightness = "25%"

Consecutively numbered brightness levels:

brightness = 4
brightness = 3
brightness = 2
brightness = 1

Floating point numbers with 1 as the max:

brightness = 1
brightness = 0.75
brightness = 0.5
brightness = 0.25

I think I like the float variant the most since it allows for future extension if the OSS firmware would add additional brightness levels (The percent based variants would need floats to represent 8 brightness levels 12.5%, etc.). But I am open to other opinions.

src/protocol.rs

src/main.rs

weberval · 2025-07-16T20:47:26Z

I haven't thought about his at all, but it makes sense. Option four seems to be the most intuitive, so I'm in favour of that, too.

* Floating point numbers with `1` as the max:
  ```toml
  brightness = 1
  brightness = 0.75
  brightness = 0.5
  brightness = 0.25
  ```

weberval · 2025-07-29T07:37:29Z

@mgjm Have you had the time to have a look at my changes?
Any additional input?

mariobehling · 2025-07-30T07:41:40Z

@weberval @mgjm Nice work. Hope this can be merged soon.

sourcery-ai bot reviewed Jun 21, 2025

View reviewed changes

src/protocol.rs Outdated Show resolved Hide resolved

mariobehling requested review from fcartegnie, CloudyPadmal and mgjm June 21, 2025 16:16

mgjm requested changes Jul 15, 2025

View reviewed changes

feat: add support for brightness control

a9b38fb

weberval force-pushed the feat/brightnesscontrol branch from 4894bde to a9b38fb Compare July 16, 2025 17:56

chore: group use statements by category

5d63cc8

weberval requested a review from mgjm July 16, 2025 18:07

SourceryAI reviewed Jul 16, 2025

View reviewed changes

sourcery-ai bot reviewed Jul 16, 2025

View reviewed changes

fossasia deleted a comment from sourcery-ai bot Jul 16, 2025

test: add u8 to Brightness test

8047e71

mgjm requested changes Jul 16, 2025

View reviewed changes

src/protocol.rs Outdated Show resolved Hide resolved

src/protocol.rs Outdated Show resolved Hide resolved

src/protocol.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

fix: remove obsolete Option wrap

ce1faf8

mgjm mentioned this pull request Jul 16, 2025

Brightness control via legacy format fossasia/badgemagic-firmware#97

Open

weberval added 3 commits July 16, 2025 22:07

chore: clearer initialisation of an empty header

cc32d54

test: fix creation of PayloadBuffer

cd31980

test: simplify test for brightness

9803b7e

weberval added 2 commits July 16, 2025 22:49

chore: cargo fmt

caf95f5

feat: switch to floating point numbers for brightness config

9594fe6

weberval requested a review from mgjm July 18, 2025 20:04

fix: error in Brightness → f32 conversion

210dc5f

weberval changed the title ~~feat: add brightnesscontrol~~ feat: add brightnesscontrol @sourcery-ai Jul 20, 2025

sourcery-ai bot changed the title ~~feat: add brightnesscontrol @sourcery-ai~~ Add display brightness control Jul 20, 2025

Add display brightness control #83

Are you sure you want to change the base?

Add display brightness control #83

Uh oh!

Conversation

weberval commented Jun 21, 2025 • edited by sourcery-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

weberval commented Jul 10, 2025

Uh oh!

mgjm commented Jul 10, 2025

Uh oh!

mgjm commented Jul 15, 2025

Uh oh!

weberval commented Jul 15, 2025

Uh oh!

mgjm commented Jul 15, 2025

Uh oh!

weberval commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgjm commented Jul 15, 2025

Uh oh!

mgjm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgjm commented Jul 15, 2025

Uh oh!

weberval commented Jul 16, 2025

Uh oh!

weberval commented Jul 16, 2025

Uh oh!

SourceryAI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SourceryAI Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

mgjm Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

mgjm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weberval commented Jul 16, 2025

Uh oh!

weberval commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariobehling commented Jul 30, 2025

Uh oh!

Uh oh!

weberval commented Jun 21, 2025 •

edited by sourcery-ai bot

Loading

weberval commented Jul 15, 2025 •

edited

Loading

weberval commented Jul 29, 2025 •

edited

Loading