Skip to content

feat(pro): added support for initializing the fans to a new mode#474

Merged
jonasmalacofilho merged 11 commits intomainfrom
commander-pro-init-fans
Oct 17, 2022
Merged

feat(pro): added support for initializing the fans to a new mode#474
jonasmalacofilho merged 11 commits intomainfrom
commander-pro-init-fans

Conversation

@MarshallAsch
Copy link
Member

@MarshallAsch MarshallAsch commented Jul 6, 2022

Related: #472

This will add some cli flags to enable / disable / change fan modes on a commander pro device

TODO:

  • update MAN pages
  • update documentation
  • update completion script
  • add / check tests

Checklist:

  • Adhere to the development process
  • Conform to the style guide
  • Verify that the changes work as expected on real hardware
  • Add automated tests cases
  • Verify that all (other) automated tests (still) pass
  • Update the README and other applicable documentation pages
  • Update the liquidctl.8 Linux/Unix/Mac OS man page
  • Update or add applicable docs/*guide.md device guides
  • Submit relevant data, scripts or dissectors to https://github.com/liquidctl/collected-device-data

New CLI flag?

  • Adjust the completion scripts in extra/completions/

New device?

  • Regenerate extra/linux/71-liquidctl.rules (instructions in the file header)
  • Add entry to the README's supported device list with applicable notes (at least en)

New driver?

  • Document the protocol in docs/developer/protocol/

@MarshallAsch MarshallAsch added the design phase In discussion/design phase label Jul 6, 2022
@jonasmalacofilho jonasmalacofilho marked this pull request as draft August 23, 2022 15:21
@MarshallAsch
Copy link
Member Author

Re: #472 (comment)

  • Change fan mode option to --fan-mode=<channel>:<mode>[,...] (e.g. --fan-mode=4:auto,5:dc,6:off)? Also, by keeping the syntax fairly generic (<key>:<value>[,...]), its parser could be useful for other future features.

@MarshallAsch MarshallAsch force-pushed the commander-pro-init-fans branch from 1c3465e to f14fcda Compare September 19, 2022 18:08
@MarshallAsch MarshallAsch marked this pull request as ready for review September 27, 2022 02:52
@MarshallAsch MarshallAsch removed the design phase In discussion/design phase label Sep 27, 2022
self._hwmon.driver)

if fan_modes:
_LOGGER.warning('kernel driver does not support the `fan mode` options, ignoring')
Copy link
Member

Choose a reason for hiding this comment

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

FYI I might still tweak this message, for consistency with other hwmon drivers and docs/developer/hwmon.md.

raise ValueError(f'invalid direction: {direction!r}')


def fan_mode_parser(value: Optional[str], max_fans: Optional[int] = None) -> dict:
Copy link
Member

Choose a reason for hiding this comment

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

After merging this, we should look into adding more type annotations, and checking them (again).

IIRC one issue is our use of:

class Base:
    def some_method(arg1, arg2, **kwargs):
        raise NotImplementedError()

and each subclass having their implementations with slightly different signatures:

def some_method(arg1, arg2, arg3=None, arg4=None, **kwargs):
    # ...

(If we end up deciding to continue not using type annotations, they should then be removed from this function).

@jonasmalacofilho jonasmalacofilho merged commit c03b445 into main Oct 17, 2022
@jonasmalacofilho
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants