API Refactoring #21

Holzhaus · 2024-02-14T22:44:23Z

This is a rather big API change, but it has several advantages: Instead of using free functions, the API becomes more straightforward to use. Also, we don't query all device properties when enumerating the connected devices.

Furthermore, all serde-related stuff is moved to main.rs as library user may not need them. The last commit may need some discussion, because now the cli feature flag needs to be passed explicitly to build the binary target (cargo run --features=cli). An alternative would be to use a cargo workspace and move the binary to a dedicated crate.

This introduces a rust-y API with two main objects: - `Device` represents a device that has not been opened yet - `DeviceHandle` is a device that has been opened and which has methods to get/set its state. Also, all serde-related stuff has been moved to `main.rs` since it's not needed for the library itself. This opens the possibility to reduce the dependencies of the library even more in the future.

timrogers · 2024-02-15T07:35:16Z

The last commit may need some discussion, because now the cli feature flag needs to be passed explicitly to build the binary target (cargo run --features=cli). An alternative would be to use a cargo workspace and move the binary to a dedicated crate.

Where is this part configuration? I was expecting to see the "feature declaration" in the code, but I can't see anything like that.

timrogers

Thanks for this great contribution! ❤️ I love the direction, but I have a few API questions.

src/lib.rs

timrogers · 2024-02-15T07:41:50Z

src/lib.rs

+
+    /// Opens the device and returns a [`DeviceHandle`] that can be used for getting and setting the
+    /// device status.
+    pub fn open(&self, api: &HidApi) -> HidResult<DeviceHandle> {


From my perspective as a Rust newbie, it fails a bit sad from an API perspective to be forced to pass the HidApi here and in other public functions.

What do you think?

timrogers · 2024-02-15T07:43:48Z

src/lib.rs

 }

-fn generate_is_on_bytes(device_type: &DeviceType) -> [u8; 20] {
+fn generate_is_enabled_bytes(device_type: &DeviceType) -> [u8; 20] {


I personally prefer the on/pff framing because it more naturally matches someone's mental model of a light.

What was your thinking behind changing this?

I changed it because there were two different functions to generate bytes for setting the state even though the only differ in one byte. And I found generate_turn_on_bytes misleading if the same function can also be used to generate turn off bytes.

I adapted the other function names for consistency. How about is_powered/set_powered? Or is_on/set_on? We can of course add convenience methods like turn_on that just call set_powered(true) internally if you want.

Can we go for is_on/set_on please? With that change, I'd be happy to merge.

timrogers · 2024-02-15T07:44:50Z

src/main.rs

@@ -97,125 +95,177 @@ fn multiples_within_range(multiples_of: u16, start_range: u16, end_range: u16) -
        .collect()
 }

+fn get_devices_by_serial_no<'a>(


Can we use the full term "serial number" here? I'd prefer not to elide things, just for better searchability.

Yup, I'll change it. I wanted to make the method name a bit shorter but I agree that searchability is important.

src/main.rs

src/lib.rs

timrogers · 2024-02-15T10:41:13Z

src/lib.rs

+    }
+
+    /// Queries the current power status of the device. Returns `true` if the device is currently on.
+    pub fn is_enabled(&self) -> HidResult<bool> {


I think I'd prefer to stick with the "on/off" paradigm rather than the "enabled" paradigm, because it matches more closely with the mental model of a light. What do you think?

Holzhaus · 2024-02-16T20:22:51Z

The last commit may need some discussion, because now the cli feature flag needs to be passed explicitly to build the binary target (cargo run --features=cli). An alternative would be to use a cargo workspace and move the binary to a dedicated crate.

Where is this part configuration? I was expecting to see the "feature declaration" in the code, but I can't see anything like that.

sorry, i forgot to push that commit :D Here it is: 42260cd

timrogers · 2024-02-18T15:54:47Z

@Holzhaus Is this done from your perspective?

Does it make sense for me to update the CI workflows to automatically include the "cli" feature?

For library usage, you can skip the unneeded dependencies by specifing `litra` as dependency in your `Cargo.toml` like this: [dependencies] litra = { version = "<version>", default-features = false }

Holzhaus · 2024-02-18T21:01:10Z

@Holzhaus Is this done from your perspective?

Yes. But there are still some open review comments (e.g., #21 (comment)). I'd appreciate your feedback unless you consider them resolved.

Does it make sense for me to update the CI workflows to automatically include the "cli" feature?

In 374e8f8 I configued the cli feature as default feature. That way, litra can be build as before (without specifying a feature explicitly). For library usage, this will require default-features = false in the dependency declaration to skip the uneeded depenencies like serde and so on.

timrogers · 2024-02-19T11:59:40Z

src/lib.rs

 }

-fn generate_is_on_bytes(device_type: &DeviceType) -> [u8; 20] {
+fn generate_is_enabled_bytes(device_type: &DeviceType) -> [u8; 20] {


Can we go for is_on/set_on please? With that change, I'd be happy to merge.

Holzhaus · 2024-02-19T21:08:47Z

I have another small patch in the pipeline to move the range checks into lib.rs instead of main.rs. Will push it soon.

timrogers · 2024-02-19T23:12:05Z

Just re-tag me for review when you're ready! 😊 Thanks for your hard work on this.

This should not be done in checks for PRs. Instead, this is better being taken care of by dependabot or something similar.

Holzhaus · 2024-02-20T18:15:35Z

This is ready now (once the checks pass). This has grown a bit, but it now has proper error handling for the entire main.rs and there are no unwrap() calls anymore (except for the two that can never fail due to the surrounding match statement). Hence, the CLI tool should never panic (unless some dependency panics).

Holzhaus added 5 commits February 14, 2024 20:52

Use more descriptive device_type_from_product_id function name

d00a36c

Increase clippy warning level and fix warnings

e38fcd4

Enforce and add documentation of all public library members

f7037cf

Move serial number filtering to main.rs

63f0b54

timrogers requested changes Feb 15, 2024

View reviewed changes

build: Mark binary-only dependencies as optional

42260cd

Holzhaus added 4 commits February 16, 2024 21:51

Expose device info of Device

39bfb10

Rework device serial number filtering code

3f941ef

Add Litra object to encapsulate the hidapi context

a5b67d4

Use more efficient find() instead of filter().next()

6e12b4c

build: Mark cli as default feature for easy binary building

374e8f8

For library usage, you can skip the unneeded dependencies by specifing `litra` as dependency in your `Cargo.toml` like this: [dependencies] litra = { version = "<version>", default-features = false }

timrogers requested changes Feb 19, 2024

View reviewed changes

timrogers mentioned this pull request Feb 19, 2024

Add auto-toggle command that turns light on/off when using a webcam #18

Merged

Holzhaus added 2 commits February 19, 2024 19:41

Merge branch 'main' of github.com:timrogers/litra-rs into api-refactor

716befe

refactor: Rename "enabled" to "on"

fc87b9e

Holzhaus added 2 commits February 19, 2024 22:52

Move range checks into lib.rs and improve library error handling

c054fac

Streamline error handling in main.rs

980b81f

Holzhaus added 4 commits February 20, 2024 18:54

build: Bump versions in Cargo.lock

62d6846

Remove unnecessary import in lib.rs

859bf40

ci: Remove Cargo.lock from pre-commit GitHub Actions workflow

7af3ec0

This should not be done in checks for PRs. Instead, this is better being taken care of by dependabot or something similar.

Add error handling for brightness calculation errors to main.rs

c82c69a

timrogers approved these changes Feb 20, 2024

View reviewed changes

timrogers merged commit 3da7a5e into timrogers:main Feb 20, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Refactoring #21

API Refactoring #21

Holzhaus commented Feb 14, 2024

timrogers commented Feb 15, 2024

timrogers left a comment

timrogers Feb 15, 2024

timrogers Feb 15, 2024

timrogers Feb 15, 2024

Holzhaus Feb 15, 2024

timrogers Feb 19, 2024

timrogers Feb 15, 2024

Holzhaus Feb 15, 2024

timrogers Feb 15, 2024

Holzhaus commented Feb 16, 2024

timrogers commented Feb 18, 2024

Holzhaus commented Feb 18, 2024

timrogers Feb 19, 2024

Holzhaus commented Feb 19, 2024

timrogers commented Feb 19, 2024

Holzhaus commented Feb 20, 2024 •

edited

Loading

API Refactoring #21

API Refactoring #21

Conversation

Holzhaus commented Feb 14, 2024

timrogers commented Feb 15, 2024

timrogers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Feb 16, 2024

timrogers commented Feb 18, 2024

Holzhaus commented Feb 18, 2024

Choose a reason for hiding this comment

Holzhaus commented Feb 19, 2024

timrogers commented Feb 19, 2024

Holzhaus commented Feb 20, 2024 • edited Loading

Holzhaus commented Feb 20, 2024 •

edited

Loading