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

driver/powerdriver: ykushpower: board_name support #1280

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PaulVittorino
Copy link
Contributor

@PaulVittorino PaulVittorino commented Oct 10, 2023

Description
When 6425df9 switched from pykush to ykushcmd, support for the YKUSH 3 and YKUSH XS was lost. Restore support for those boards by passing the board_name argument to ykushcmd. The board_name is determined by listing the attached YKUSH boards by model.

Checklist

  • Documentation for the feature
  • Tests for the feature
  • The arguments and description in doc/configuration.rst have been updated
  • PR has been tested

@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 62.2%. Comparing base (722838b) to head (48bd190).
Report is 5 commits behind head on master.

❗ Current head 48bd190 differs from pull request most recent head c9228a9. Consider uploading reports for the commit c9228a9 to get more accurate results

Files Patch % Lines
labgrid/driver/powerdriver.py 88.8% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1280   +/-   ##
======================================
  Coverage    62.2%   62.2%           
======================================
  Files         164     164           
  Lines       12191   12207   +16     
======================================
+ Hits         7584    7599   +15     
- Misses       4607    4608    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jluebbe
Copy link
Member

jluebbe commented Oct 13, 2023

Isn't it possible to detect the model automatically (i.e. using the USB VID/PID) instead of requiring manual configuration by the user? How did this work previously? @edersondisouza?

@PaulVittorino
Copy link
Contributor Author

Isn't it possible to detect the model automatically (i.e. using the USB VID/PID) instead of requiring manual configuration by the user?

The user manuals for the YKUSH XS, YKUSH, and YKUSH3 state the products have the same VID but different PIDs. Therefore, I believe it is possible to detect the model automatically. Would you advise changing YKUSHPowerPort to inherit from USBResource?

How did this work previously?

pykush does not have a board_type argument.

$ python3.8 pykush.py -l
listing YKUSH family devices
  found a YKUSH XS release 2 device with serial number YKU2768
    system device path 1-8.2:1.0, vendor id 0x04d8, product id 0xf0cd
    the device has 1 downstream port
    Checking running power state, port 1 : UP
  found a YKUSH3 release 1 device with serial number Y3N10673
    system device path 1-4.4:1.0, vendor id 0x04d8, product id 0xf11b
    the device is running a v1.0 firmware and has 3 downstream ports
    downstream running power states, port 1 to 3: UP, UP, UP

ykushcmd requires the board_name argument for the YKUSH XS and YKUSH3 boards.

$ ./bin/ykushcmd -l
Attached YKUSH Boards:
No YKUSH boards found.

$ ./bin/ykushcmd ykush3 -l
Attached YKUSH3 Boards:
1. Board found with serial number: Y3N10673

$ ./bin/ykushcmd ykushxs -l
Attached YKUSH XS Boards:
1. Board found with serial number: YKU2768

@edersondisouza
Copy link
Contributor

Ouch, this is really inconvenient =/

Wonder if trying to get USB PID will work nice with the remote instance. Maybe require pykush and use the command line tool (not the module, so it's easy to support remote instance)?

An ugly hack would be to try to chain three calls to ykushcmd and use it, like ykushcmd ... | ykushcmd ykush3 ... | ykushcmd ykushxs... (/me hides in the corner).

Or fix ykushcmd itself... =/

@Bastian-Krause Bastian-Krause added the needs author info Requires more information from the PR/Issue author label Nov 6, 2023
@jluebbe
Copy link
Member

jluebbe commented Dec 15, 2023

Fixing ykushkmd would be nice and should be easy based on the VID/PID info.

Otherwise, you could make YKUSHPowerPort inherit from USBResource, which will give it additional USB properties (busnum, devnum, path, vendor_id and model_id). NetworkYKUSHPowerPort would then inherit from RemoteUSBResource to store these new properties.

YKUSHPowerPort would match USB HID devices by the given serial. In the Driver, you could then use the USB vendor/model IDs to generate the correct board name for ykushcmd.

The additional resource properties would break backwards compatibility, but the current state is broken anyway. :/

@PaulVittorino
Copy link
Contributor Author

The additional resource properties would break backwards compatibility, but the current state is broken anyway. :/

I found a way to not break backward compatibility. Use ykushcmd to list out the attached boards by model. Then determine the model of a given board by looking for the serial number in the output for each model. It will require adding each new board to the YKUSHPowerDriver but the PID/VID solution had the same issue.

doc/configuration.rst Outdated Show resolved Hide resolved
labgrid/driver/powerdriver.py Show resolved Hide resolved
Copy link
Contributor

@edersondisouza edersondisouza left a comment

Choose a reason for hiding this comment

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

Nice workaround! Minor nit: on commit message the phrase "Added tests for YKUSHPowerPort and YKUSHPowerPort.' seems wrong - maybe you meant YKUSHPowerDriver for the second YKUSHPowerPort?

@PaulVittorino
Copy link
Contributor Author

Nice workaround! Minor nit: on commit message the phrase "Added tests for YKUSHPowerPort and YKUSHPowerPort.' seems wrong - maybe you meant YKUSHPowerDriver for the second YKUSHPowerPort?

Yes, thanks for catching that.

When 6425df9 switched from pykush to
ykushcmd, support for the YKUSH 3 and YKUSH XS was lost. Restore support
for those boards by passing the board_name argument to ykushcmd. The
board_name is determined by listing the attached YKUSH boards by model.

Added tests for YKUSHPowerPort and YKUSHPowerDriver.

Signed-off-by: Paul Vittorino <paul.vittorino@garmin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs author info Requires more information from the PR/Issue author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants