Skip to content

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

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 45.2%. Comparing base (dbd4b11) to head (6952f14).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
labgrid/driver/powerdriver.py 88.8% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1280   +/-   ##
======================================
  Coverage    45.1%   45.2%           
======================================
  Files         173     173           
  Lines       13712   13721    +9     
======================================
+ Hits         6188    6203   +15     
+ Misses       7524    7518    -6     
Flag Coverage Δ
3.10 45.2% <88.8%> (+<0.1%) ⬆️
3.11 45.2% <88.8%> (+<0.1%) ⬆️
3.12 45.2% <88.8%> (+<0.1%) ⬆️
3.13 45.1% <88.8%> (+<0.1%) ⬆️
3.14 45.1% <88.8%> (+<0.1%) ⬆️
3.9 45.2% <88.8%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

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.

@edersondisouza
Copy link
Contributor

Any more comments on this, @jluebbe and @Bastian-Krause?

@Bastian-Krause Bastian-Krause added fix and removed needs author info Requires more information from the PR/Issue author labels Feb 2, 2026
Bastian-Krause
Bastian-Krause previously approved these changes Feb 2, 2026
Copy link
Member

@Bastian-Krause Bastian-Krause left a comment

Choose a reason for hiding this comment

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

Force-pushed: rebased, fixed remote support for the model detection

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>
[bst: rebased, added self.port.command_prefix to model detection for
remote access, dropped redundant/unrelated test_import_backend_poe_mib]
Signed-off-by: Bastian Krause <bst@pengutronix.de>
@Bastian-Krause
Copy link
Member

Force-pushed: dropped unrelated copy-pasted test_import_backend_poe_mib.

@Bastian-Krause Bastian-Krause merged commit caf7833 into labgrid-project:master Feb 2, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants