Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add basic support for the Logitech Litra Beam LX #294

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

timrogers
Copy link
Owner

This pull request adds basic support for the Logitech Litra Beam LX, a new Litra device from Logitech.

The code is inspired by @lnovelli's fork of this project, where @lnovelli has already kindly worked out the new device's details and how it works 💜

For now, this just supports our existing functionality (managing the device's on/off state, brightness and temperature), and not the Litra Beam LX's coloured RGB zone.

Fixes #288.

This pull request adds basic support for the Logitech Litra Beam
LX, a new Litra device from Logitech.

The code is inspired by [@lnovelli](https://github.com/lnovelli/litra-lx)'s
fork of this project, where @lnovelli has already kindly worked
out the new device's details and how it works 💜

For now, this just supports our existing functionality (managing
the device's on/off state, brightness and temperature), and not
the Litra Beam LX's coloured RGB zone.

Fixes #288.
@timrogers
Copy link
Owner Author

timrogers commented Dec 23, 2023

@jbsparrow Would you be willing to give this a try for me? 🙏🏻

All you need to do is clone this repo and install the dependencies with npm i, and then this should be ready for testing. Could you please try the following for me?

  • Run node dist/commonjs/cli/litra-devices.js - your device should be listed as a Litra Beam LX in the "off" state
  • Run node dist/commonjs/cli/litra-on.js - your device should turn on
  • Run node dist/commonjs/cli/litra-devices.js - your device should now show in the "on" state
  • Run node dist/commonjs/cli/litra-toggle.js twice - your device should turn off, then on again
  • Run node dist/commonjs/cli/litra-brightness-lm.js 80 - your device's brightness should change
  • Run node dist/commonjs/cli/litra-temperature-k.js 3200 - your device's temperature should change

Once we've run through that quickly and things seem to be working, I'd love to merge and release this 🥳

@timrogers
Copy link
Owner Author

@lnovelli You'd also be welcome to try the steps above, if you have a chance 🙏🏻

@jbsparrow
Copy link

I'm so sorry I wasn't able to get back to you earlier. I just gave it a try and it works perfect! Would it be possible to add support for the RGB backlight at some point? I was taking a look at the code but unfortunately I really don't have any experience with node so I wasn't able to figure out.

Copy link

@jbsparrow jbsparrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and all features work perfect. I think it's good to go!

@timrogers
Copy link
Owner Author

I'm so sorry I wasn't able to get back to you earlier. I just gave it a try and it works perfect!

Thank you ❤️

Would it be possible to add support for the RGB backlight at some point? I was taking a look at the code but unfortunately I really don't have any experience with node so I wasn't able to figure out.

Yep! I definitely want to come back to this, but I wanted to do a simple implementation first.

@timrogers timrogers merged commit 31b67c5 into main Jan 8, 2024
3 checks passed
Copy link

github-actions bot commented Jan 8, 2024

🎉 This PR is included in version 4.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@timrogers
Copy link
Owner Author

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5] 3
🧪 Relevant tests Yes
🔒 Security concerns No
⚡ Key issues to review None

@timrogers
Copy link
Owner Author

timrogers commented Jun 26, 2024

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Maintainability
Refactor to use a common helper function for generating command bytes to reduce code duplication

To improve maintainability and avoid code duplication, consider refactoring the
generateTurnOnBytes, generateTurnOffBytes, generateIsOnBytes,
generateSetTemperatureInKelvinBytes, and generateSetBrightnessInLumenBytes functions to
use a common helper function that takes the device.type and the specific command bytes as
parameters.

dist/esm/driver.js [80-103]

-const generateTurnOnBytes = (device) => {
-    if (device.type === DeviceType.LitraBeamLX) {
-        return padRight([0x11, 0xff, 0x06, 0x1c, 0x01], 20, 0x00);
-    }
-    else {
-        return padRight([0x11, 0xff, 0x04, 0x1c, 0x01], 20, 0x00);
-    }
+const generateCommandBytes = (device, commandBytes) => {
+    const typeSpecificByte = device.type === DeviceType.LitraBeamLX ? 0x06 : 0x04;
+    return padRight([0x11, 0xff, typeSpecificByte, ...commandBytes], 20, 0x00);
 };
-const generateTurnOffBytes = (device) => {
-    if (device.type === DeviceType.LitraBeamLX) {
-        return padRight([0x11, 0xff, 0x06, 0x1c, 0x00], 20, 0x00);
-    }
-    else {
-        return padRight([0x11, 0xff, 0x04, 0x1c, 0x00], 20, 0x00);
-    }
-};
+const generateTurnOnBytes = (device) => generateCommandBytes(device, [0x1c, 0x01]);
+const generateTurnOffBytes = (device) => generateCommandBytes(device, [0x1c, 0x00]);
 
Suggestion importance[1-10]: 9

Why: This suggestion significantly improves maintainability by reducing code duplication and centralizing the logic for generating command bytes. This makes the code easier to read and maintain.

9
Refactor similar functions into a single function to reduce code duplication

The generateTurnOnBytes, generateTurnOffBytes, generateIsOnBytes,
generateSetTemperatureInKelvinBytes, generateGetTemperatureInKelvinBytes,
generateSetBrightnessInLumenBytes, and generateGetBrightnessInLumenBytes functions share a
common pattern. Consider refactoring them to a single function that accepts the command
and device type as parameters to reduce code duplication.

src/driver.ts [109-306]

-const generateTurnOnBytes = (device: Device): number[] => {
-  if (device.type === DeviceType.LitraBeamLX) {
-    return padRight([0x11, 0xff, 0x06, 0x1c, 0x01], 20, 0x00);
-  } else {
-    return padRight([0x11, 0xff, 0x04, 0x1c, 0x01], 20, 0x00);
-  }
-};
-...
-const generateGetBrightnessInLumenBytes = (device: Device): number[] => {
-  if (device.type === DeviceType.LitraBeamLX) {
-    return padRight([0x11, 0xff, 0x06, 0x31], 20, 0x00);
-  } else {
-    return padRight([0x11, 0xff, 0x04, 0x31], 20, 0x00);
-  }
+const generateCommandBytes = (device: Device, command: number[]): number[] => {
+  const prefix = device.type === DeviceType.LitraBeamLX ? [0x11, 0xff, 0x06] : [0x11, 0xff, 0x04];
+  return padRight([...prefix, ...command], 20, 0x00);
 };
 
+const generateTurnOnBytes = (device: Device): number[] => generateCommandBytes(device, [0x1c, 0x01]);
+const generateTurnOffBytes = (device: Device): number[] => generateCommandBytes(device, [0x1c, 0x00]);
+const generateIsOnBytes = (device: Device): number[] => generateCommandBytes(device, [0x01]);
+const generateSetTemperatureInKelvinBytes = (device: Device, temperatureInKelvin: number): number[] => generateCommandBytes(device, [0x9c, ...integerToBytes(temperatureInKelvin)]);
+const generateGetTemperatureInKelvinBytes = (device: Device): number[] => generateCommandBytes(device, [0x81]);
+const generateSetBrightnessInLumenBytes = (device: Device, brightnessInLumen: number): number[] => generateCommandBytes(device, [0x4c, ...integerToBytes(brightnessInLumen)]);
+const generateGetBrightnessInLumenBytes = (device: Device): number[] => generateCommandBytes(device, [0x31]);
+
Suggestion importance[1-10]: 8

Why: This suggestion significantly improves code maintainability by reducing duplication and making the codebase easier to manage. It consolidates multiple similar functions into a single, more flexible function.

8
Refactor to use a utility function for generating bytes to reduce redundancy and improve maintainability

To improve maintainability and reduce redundancy, consider creating a utility function
generateBytes that takes the device type, command, and additional parameters. This will
eliminate the need for multiple similar functions like generateTurnOnBytes,
generateTurnOffBytes, etc.

dist/commonjs/driver.js [88-112]

-const generateTurnOnBytes = (device) => {
-    if (device.type === DeviceType.LitraBeamLX) {
-        return (0, utils_1.padRight)([0x11, 0xff, 0x06, 0x1c, 0x01], 20, 0x00);
-    }
-    else {
-        return (0, utils_1.padRight)([0x11, 0xff, 0x04, 0x1c, 0x01], 20, 0x00);
-    }
-};
-const generateTurnOffBytes = (device) => {
-    if (device.type === DeviceType.LitraBeamLX) {
-        return (0, utils_1.padRight)([0x11, 0xff, 0x06, 0x1c, 0x00], 20, 0x00);
-    }
-    else {
-        return (0, utils_1.padRight)([0x11, 0xff, 0x04, 0x1c, 0x00], 20, 0x00);
-    }
+const generateBytes = (device, command, additionalParams) => {
+    const baseCommand = device.type === DeviceType.LitraBeamLX ? [0x11, 0xff, 0x06] : [0x11, 0xff, 0x04];
+    return (0, utils_1.padRight)([...baseCommand, command, ...additionalParams], 20, 0x00);
 };
 
+const generateTurnOnBytes = (device) => generateBytes(device, 0x1c, [0x01]);
+const generateTurnOffBytes = (device) => generateBytes(device, 0x1c, [0x00]);
+
Suggestion importance[1-10]: 8

Why: This suggestion effectively reduces redundancy and improves maintainability by consolidating similar functions into a single utility function. This change simplifies the code and makes it easier to manage.

8
Extract common logic for padding bytes into a separate utility function

To enhance readability and maintainability, consider extracting the common logic for
padding bytes into a separate utility function that can be reused across different
generate functions.

dist/esm/driver.js [148-154]

 const generateSetTemperatureInKelvinBytes = (device, temperatureInKelvin) => {
-    if (device.type === DeviceType.LitraBeamLX) {
-        return padRight([0x11, 0xff, 0x06, 0x9c, ...integerToBytes(temperatureInKelvin)], 20, 0x00);
-    }
-    else {
-        return padRight([0x11, 0xff, 0x04, 0x9c, ...integerToBytes(temperatureInKelvin)], 20, 0x00);
-    }
+    return generateCommandBytes(device, [0x9c, ...integerToBytes(temperatureInKelvin)]);
 };
 
Suggestion importance[1-10]: 8

Why: This suggestion enhances readability and maintainability by centralizing the padding logic, making the code cleaner and easier to manage.

8
Create a helper function to generate the expected HID write array to reduce code duplication

To avoid code duplication and improve maintainability, consider creating a helper function
to generate the expected HID write array. This will make it easier to update the array
format in the future if needed.

tests/driver.test.ts [74-76]

-expect(fakeLitraBeamLx.hid.write).toBeCalledWith([
-  17, 255, 6, 28, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
-]);
+const expectedWriteArray = (type: number, state: number) => [
+  17, 255, type, 28, state, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+];
 
+expect(fakeLitraBeamLx.hid.write).toBeCalledWith(expectedWriteArray(6, 1));
+
Suggestion importance[1-10]: 7

Why: This suggestion improves maintainability by reducing code duplication, making future updates easier. However, it is not crucial for functionality.

7
Possible issue
Add error handling for I/O operations to manage potential communication errors with the device

Consider adding error handling for the device.hid.write and device.hid.readSync methods to
manage potential I/O errors gracefully.

src/driver.ts [124-176]

-device.hid.write(bytes);
-const data = device.hid.readSync();
+try {
+  device.hid.write(bytes);
+  const data = device.hid.readSync();
+} catch (error) {
+  console.error('Error communicating with device:', error);
+  return false; // or handle the error as needed
+}
 
Suggestion importance[1-10]: 9

Why: Adding error handling for I/O operations is crucial for robustness and reliability. This change ensures that potential communication errors with the device are managed gracefully, improving the overall stability of the code.

9
Add a test case to handle errors when the hid.write method fails

Add a test case to handle the scenario where the hid.write method fails, to ensure the
code handles such errors gracefully.

tests/driver.test.ts [71-77]

-it('sends the correct instruction to turn the device on for a Litra Beam LX', () => {
-  turnOn(fakeLitraBeamLx);
+it('handles errors when turning the device on for a Litra Beam LX', () => {
+  fakeLitraBeamLx.hid.write.mockImplementation(() => { throw new Error('Write failed'); });
   
-  expect(fakeLitraBeamLx.hid.write).toBeCalledWith([
-    17, 255, 6, 28, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
-  ]);
+  expect(() => turnOn(fakeLitraBeamLx)).toThrow('Write failed');
 });
 
Suggestion importance[1-10]: 9

Why: Adding a test case to handle potential errors is important for robustness and ensuring the code handles unexpected failures gracefully.

9
Possible bug
Validate the device object before using its properties to ensure consistency and avoid potential bugs

To ensure consistency and avoid potential bugs, validate the device object before using
its properties in functions like generateTurnOnBytes, generateTurnOffBytes, etc.

dist/commonjs/driver.js [88-94]

 const generateTurnOnBytes = (device) => {
+    if (!device || !device.type) {
+        throw new Error("Invalid device object");
+    }
     if (device.type === DeviceType.LitraBeamLX) {
         return (0, utils_1.padRight)([0x11, 0xff, 0x06, 0x1c, 0x01], 20, 0x00);
-    }
-    else {
+    } else {
         return (0, utils_1.padRight)([0x11, 0xff, 0x04, 0x1c, 0x01], 20, 0x00);
     }
 };
 
Suggestion importance[1-10]: 9

Why: Validating the device object before using its properties is crucial for preventing potential bugs and ensuring the code handles invalid inputs gracefully. This is a significant improvement for code reliability.

9
Ensure the integerToBytes function is defined and robust against edge cases

Ensure that the integerToBytes function is defined and handles edge cases, such as
negative numbers or numbers that exceed the expected byte range.

src/driver.ts [187-195]

-[0x11, 0xff, 0x04, 0x9c, ...integerToBytes(temperatureInKelvin)],
+function integerToBytes(value: number): number[] {
+  if (value < 0 || value > 65535) {
+    throw new Error('Value out of range');
+  }
+  return [(value >> 8) & 0xff, value & 0xff];
+}
 
Suggestion importance[1-10]: 7

Why: Ensuring that the integerToBytes function handles edge cases is important for preventing potential bugs. This suggestion improves the robustness of the code, although it assumes the function is not already defined elsewhere.

7
Best practice
Reset mock implementations after each test to avoid side effects

Ensure that the hid.readSync mock implementation is reset after each test to avoid
potential side effects between tests.

tests/driver.test.ts [132-134]

+afterEach(() => {
+  jest.resetAllMocks();
+});
+
 fakeLitraBeamLx.hid.readSync = jest
   .fn()
   .mockReturnValue([17, 255, 4, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]);
 
Suggestion importance[1-10]: 8

Why: Resetting mocks after each test is a best practice to prevent side effects between tests, ensuring test isolation and reliability.

8
Use a specific error type for better error handling

To improve error handling, consider using a more specific error type (e.g., Error) instead
of throwing a string when the provided temperature or brightness is out of range.

dist/esm/driver.js [173]

-throw `Provided temperature must be a multiple of 100 between ${minimumTemperature} and ${maximumTemperature} for this device`;
+throw new Error(`Provided temperature must be a multiple of 100 between ${minimumTemperature} and ${maximumTemperature} for this device`);
 
Suggestion importance[1-10]: 8

Why: Using a specific error type like Error improves error handling and debugging, making it clearer what kind of error is being thrown.

8
Use a custom error class for device-related errors to improve error handling

To improve error handling, consider using a custom error class for device-related errors
instead of throwing generic strings. This will make it easier to catch and handle specific
errors.

dist/commonjs/driver.js [185]

-throw `Provided temperature must be a multiple of 100 between ${minimumTemperature} and ${maximumTemperature} for this device`;
+class DeviceError extends Error {
+    constructor(message) {
+        super(message);
+        this.name = "DeviceError";
+    }
+}
 
+throw new DeviceError(`Provided temperature must be a multiple of 100 between ${minimumTemperature} and ${maximumTemperature} for this device`);
+
Suggestion importance[1-10]: 7

Why: Using a custom error class improves error handling by making it easier to catch and handle specific errors, enhancing code robustness and maintainability.

7
Use a constant for the padding length to improve code readability and maintainability

Consider using a constant for the padding length (20) used in the padRight function to
improve code readability and maintainability.

src/driver.ts [111]

-return padRight([0x11, 0xff, 0x06, 0x1c, 0x01], 20, 0x00);
+const PADDING_LENGTH = 20;
+return padRight([0x11, 0xff, 0x06, 0x1c, 0x01], PADDING_LENGTH, 0x00);
 
Suggestion importance[1-10]: 6

Why: Using a constant for the padding length enhances code readability and maintainability. While this is a good practice, it is a minor improvement compared to the other suggestions.

6
Performance
Cache the result of HID.devices() to improve performance

To improve performance, consider caching the result of HID.devices() if the device list is
not expected to change frequently, to avoid repeatedly querying the HID devices.

dist/esm/driver.js [77]

-const matchingDevices = HID.devices().filter(isLitraDevice);
+const cachedDevices = HID.devices();
+const matchingDevices = cachedDevices.filter(isLitraDevice);
 
Suggestion importance[1-10]: 7

Why: Caching the result of HID.devices() can improve performance by reducing redundant queries, but it should be implemented carefully to ensure the cache remains valid.

7
Enhancement
Use a loop to iterate over different device types and their expected HID write arrays to reduce redundancy

To improve readability and reduce redundancy, consider using a loop to iterate over the
different device types and their expected HID write arrays in the turnOn and turnOff test
cases.

tests/driver.test.ts [55-77]

-it('sends the correct instruction to turn the device on for a Litra Glow', () => {
-  turnOn(fakeLitraGlow);
-  
-  expect(fakeLitraGlow.hid.write).toBeCalledWith([
-    17, 255, 4, 28, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
-  ]);
+const devices = [
+  { device: fakeLitraGlow, type: 4 },
+  { device: fakeLitraBeam, type: 4 },
+  { device: fakeLitraBeamLx, type: 6 },
+];
+
+devices.forEach(({ device, type }) => {
+  it(`sends the correct instruction to turn the device on for type ${type}`, () => {
+    turnOn(device);
+    
+    expect(device.hid.write).toBeCalledWith([
+      17, 255, type, 28, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+    ]);
+  });
 });
 
Suggestion importance[1-10]: 6

Why: This suggestion enhances readability and reduces redundancy, but it is more of a code style improvement rather than a critical change.

6
Readability
Use a switch statement instead of multiple if-else blocks to enhance code readability

To enhance code readability, consider using a switch statement instead of multiple if-else
blocks when generating bytes based on device type.

dist/commonjs/driver.js [88-94]

 const generateTurnOnBytes = (device) => {
-    if (device.type === DeviceType.LitraBeamLX) {
-        return (0, utils_1.padRight)([0x11, 0xff, 0x06, 0x1c, 0x01], 20, 0x00);
-    }
-    else {
-        return (0, utils_1.padRight)([0x11, 0xff, 0x04, 0x1c, 0x01], 20, 0x00);
+    switch (device.type) {
+        case DeviceType.LitraBeamLX:
+            return (0, utils_1.padRight)([0x11, 0xff, 0x06, 0x1c, 0x01], 20, 0x00);
+        default:
+            return (0, utils_1.padRight)([0x11, 0xff, 0x04, 0x1c, 0x01], 20, 0x00);
     }
 };
 
Suggestion importance[1-10]: 5

Why: While using a switch statement can enhance readability, the improvement is minor. The existing if-else structure is already clear and concise.

5

timrogers added a commit to eviltimrogers/litra that referenced this pull request Aug 2, 2024
This pull request adds basic support for the Logitech Litra Beam
LX, a new Litra device from Logitech, which has product ID
`0xc903`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not work with Logitech Litra Beam LX
2 participants