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

Commander core update #501

Merged
merged 3 commits into from
Sep 13, 2022

Conversation

Meriemi-git
Copy link
Contributor

@Meriemi-git Meriemi-git commented Aug 30, 2022

Add support for the last Corsair Commander Core firmware v2.10.219.
I follow the explanation of ParkerMC to fix the issue, I changed the flow inside the _read_data and _write_data functions to follow the flow described by ParkerMC : set_mode > read / write > reset

I changed the read operation inside _send_command function to read until response starts with 0x00.

After changes to the command flow the tests failed with an "Key Error" at line 117 of file test_commander_core.py.
This is because when i delayed de reset command in read_data and when we start with a set_mode command the _modes array of the Mock device is empty. To prevent this i set in init a None value inside the _modes array.

Fixes: #464


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/

@Meriemi-git
Copy link
Contributor Author

Meriemi-git commented Aug 30, 2022

I have test this on my Corsair H150i Elite Capellix with firmware v.2.10.219, it works but i'm not very confident about the few modifications of the unit tests.

@ParkerMc
Copy link
Contributor

Fix looks good. Do you mind adding the flow to the protocol documentation?
I think for the unit test, the main thing is to verify it is closed(reset) before the mode is changed or it is set to sleep mode.

Copy link
Member

@jonasmalacofilho jonasmalacofilho left a comment

Choose a reason for hiding this comment

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

Thanks!

On top of Parker's comment, please see the two notes I left.

@@ -30,7 +30,7 @@ def __init__(self):
self.sent = list()

self._last_write = bytes()
self._modes = {}
self._modes = [None]
Copy link
Member

Choose a reason for hiding this comment

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

To me it seems that the issue is that

# In MockCommanderCoreDevice.write():
if self._modes[channel] is None:

should have always been

if self._modes.get(channel) is None:

What do you think, @ParkerMc?


Either way, initializing a dict with a list doesn't feel right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonasmalacofilho Thanks for your feedback you're right it seems to be a better solution to leave the dict empty. I tested with your suggestion to get the mode and it works.

.gitignore Outdated
.DS_Store
.vscode
Copy link
Member

Choose a reason for hiding this comment

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

Please use a personal global gitignore file for editor files, given that they are a personal and therefore vary varied choice. See gitignore for more information, especially the core.excludesFile part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's done.

    Inside _send_command, read until response doesn't start by 0x00
@Meriemi-git Meriemi-git force-pushed the commander_core_update branch 2 times, most recently from 223b11f to c951be9 Compare August 31, 2022 09:22
@Meriemi-git
Copy link
Contributor Author

Meriemi-git commented Aug 31, 2022

@ParkerMc I updated the write function in test to ensure mode is not set whereas previous was not reset. I had to ensure device initially call a reset because of the command flow modification (reset is not the first command anymore) .
I put the initial reset call in the _wake_device_context function but i hesitated between here and in the init function.

I don't see any section in protocol documentation about the command flow but i can create a new one to document this changed for all new firmware versions from v2.10.219

@ParkerMc
Copy link
Contributor

ParkerMc commented Aug 31, 2022

I put the initial reset call in the _wake_device_context function but i hesitated between here and in the init function.

Is that necessary? From my testing, reset is only needed after a write or read.
Hense why I proposed renaming it so it makes more sense (see below)

It should become: open endpoint(currently called set_mode) -> read and/or write -> close endpoint(currently called reset)

I don't see any section in protocol documentation about the command flow but i can create a new one to document this changed for all new firmware versions from v2.10.219

This is the proper use of the protocol so it applies to v1 and v2. I just had the flow wrong but it still worked.

Rename functions
Update protocol documentation.
@Meriemi-git
Copy link
Contributor Author

@ParkerMc You're right, i remove this unnecessary call and update the protocol doc.

@jonasmalacofilho
Copy link
Member

@ParkerMc does everything here look good to you?

@ParkerMc
Copy link
Contributor

@ParkerMc does everything here look good to you?

Yeah, looks great. Sorry, I thought I had replied the other day.

@jonasmalacofilho
Copy link
Member

Great! Thank you both!

@jonasmalacofilho jonasmalacofilho merged commit a2b4391 into liquidctl:main Sep 13, 2022
jonasmalacofilho added a commit that referenced this pull request Oct 9, 2022
Related: #455 ("Corsair Commander Core Error, Incorrect Data Type")
Related: #464 ("Corsair Commander Core giving error: 'response does not match command'")
Related: PR #501 ("Commander core update")
Related: #520 ("Corsair Commander Core Error: Incorrect Data Type (Even with the newest codes)")
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.

Corsair Commander Core giving error: 'response does not match command'
3 participants