IniterWorker / gc9a01

GC9A01 Display Driver
Apache License 2.0
16 stars 7 forks source link

Allow access to raw brightness value #6

Closed chrisprice closed 1 month ago

chrisprice commented 2 months ago

Thanks for the driver, it's been a great head start when trying to make use of this device.

Would it be possible to expose access to the inner brightness value?

For context, I'm trying to hack together an async version of the driver so that I can efficiently use it with embassy. I've got it mostly working but at the minute I'm forced use raw brightness values e.g. -

command_wrapper
        .send(Command::DisplayBrightness(0x5F))
        .unwrap();

An alternative change (happy to make it), would be to change DisplayBrightness to accept Brightness if you'd prefer that?

IniterWorker commented 2 months ago

Would it be possible to expose access to the inner brightness value?

To accommodate your request to access the inner brightness value, I suggest using the pub const fn custom(raw: u8) method. This function will allow you to set a custom brightness value directly.

As per the documentation:

It should be checked what is the relationship between this written value and output brightness of the display. This relationship is defined on the display module specification. In principle, the relationship is that 00h value means the lowest brightness and FFh value means the highest brightness.

You can use this method to create custom brightness values, understanding that the relationship between the raw value and the output brightness depends on the display module's specification.

Task(s):

  1. Expose the const fn custom(...) -> Self as pub.
  2. Update the documentation of pub const fn custom(...) to reflect the above documentation quote.

This approach will give you the flexibility you need without exposing private members.

chrisprice commented 2 months ago

/aside that's a very bot like response...

Your suggestion is the opposite of what this code is trying to achieve. I'm looking to access the value once constructed.

IniterWorker commented 2 months ago

So, your request doesn't align with the design of this driver/hardware. Brightness is a public encapsulation of a private protocol implementation. Since it's not possible to read directly from the hardware, exposing private members would create an error-prone interface.

Tip: You should manage this state at your application level.

Thanks for your contribution. I will expose Brightness::custom in release 0.3.0.

Please, see PR Release 0.3.0

chrisprice commented 2 months ago

Thanks but I think we're still talking cross-purposes. I'm explicitly not trying to read back the brightness of the device.

I'm reusing the publicly exposed innards to create a bespoke driver.

I would like to recreate the functionality of Driver::set_brightness in my driver. To that end, I'm invoking Command::send on Command::Brightness (both public).

The implementation of Driver::set_brightness is as follows -

    pub fn set_brightness(&mut self, brightness: Brightness) -> Result<(), DisplayError> {
        Command::DisplayBrightness(brightness.brightness).send(&mut self.interface)
    }

However, I can't recreate this function because there is no public access to Brightness::brightness.

Therefore, I would like to expose it as public or change Command::DisplayBrightness to accept the Brightness directly and unwrap it within Command::send (having gone around the loop a bit on this now I think my preference is for the latter).

Such a change would satisfy your concern about not permitting the value to be read back. It also feels cleanest to unwrap a newtype as close as possible to the system boundary.

IniterWorker commented 1 month ago

@chrisprice, the value is now available via Brightness::brightness.

For context, I'm trying to hack together an async version of the driver so that I can efficiently use it with embassy. I've got it mostly working but at the minute I'm forced use raw brightness values e.g. -

With the help of maybe_async_cfg crate, the async version will come with the 0.5.0 version of this driver.