feat(gui): fans can have multiple rpm values and add "fan_multi" #2016
Conversation
… has multiple rpm vlaues
dosubot
bot
added
the
size:S
Reviewer's Guide by SourceryThis pull request implements support for fans with multiple RPM values and adds the "fan_multi" type. The changes primarily affect the display of fan RPM in the UI and the handling of different fan types in the store. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @xPatrikTwitch - I've reviewed your changes - here's some feedback:
Overall Comments:
- Could you provide more documentation on the 'fan_multi' type and its relationship to the Klipper firmware? This would help clarify the use case and benefits of this feature.
- Please add some tests to ensure that existing fan configurations are not affected by these changes and that the new multi-RPM functionality works as expected.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| <small v-if="rpm !== null && !Array.isArray(rpm)" :class="rpmClasses"> | ||
| {{ Math.round(rpm ?? 0) }} RPM | ||
| </small> | ||
| <template v-if="Array.isArray(rpm)"> |
There was a problem hiding this comment.
suggestion: Consider extracting RPM display logic to improve readability
The new logic for displaying multiple RPM values adds complexity to the template. Consider extracting this into a separate component or computed property to improve readability and maintainability.
<template v-if="isRpmArray">
| @@ -186,10 +186,10 @@ export const getters: GetterTree<PrinterState, RootState> = { | |||
|
|
|||
| getFans: (state, getters) => { | |||
| const fans: PrinterStateFan[] = [] | |||
| const supportedFans = ['temperature_fan', 'controller_fan', 'heater_fan', 'fan_generic', 'fan'] | |||
There was a problem hiding this comment.
question: Clarify the distinction between 'fan_multi' and 'fan_generic'
Can you provide more information about the 'fan_multi' type and how it differs from 'fan_generic'? Understanding the distinction would help clarify the purpose of this addition.
|
@xPatrikTwitch pls ignore the bot. we are testing it right now... will the module fan_multi be merged in upstream Klipper? |
There is a pull request open Klipper3d/klipper#6691 |
Description
Changes how the fan rpm is displayed so it can be an array of multiple rpm values and also adds support "fan_multi" a fan that can have multiple tachometer_pins defined (https://github.com/xPatrikTwitch/klipper/blob/master/klippy/extras/fan_multi.py) (This does not break normal fans with one rpm value)
Related Tickets & Documents
none
Mobile & Desktop Screenshots/Recordings
[optional] Are there any post-deployment tasks we need to perform?
none
Summary by Sourcery
Add support for fans with multiple RPM values and introduce the 'fan_multi' type, enhancing the GUI to display these values appropriately.
New Features:
Enhancements: