labgrid icon indicating copy to clipboard operation
labgrid copied to clipboard

YKUSH set pykush in requierments and make available for remote access

Open mbgg opened this issue 4 years ago • 12 comments

Description

This pull requests has two important changes.

  1. The ykush pypip implementation [2] lacks basic functionality like for example connect to several Yepkits via the serial number. Make sure to use the official [2] pykush implementation by adding the dependency in the requierements.txt

  2. Make YKUSH accessible through remote access. With this patch we can export the access to the YKUSH and toggle the power via the labgrid-client interface.

[1] https://github.com/Yepkit/pykush [2] https://github.com/alvarop/pykush/

Checklist

  • [ ] Documentation for the feature
  • [ ] Tests for the feature
  • [ ] The arguments and description in doc/configuration.rst have been updated
  • [ ] Add a section on how to use the feature to doc/usage.rst
  • [ ] Add a section on how to use the feature to doc/development.rst
  • [x] CHANGES.rst has been updated
  • [x] PR has been tested
  • [ ] Man pages have been regenerated

mbgg avatar May 19 '20 22:05 mbgg

Codecov Report

Merging #606 (6ceb945) into master (02d5b1b) will increase coverage by 0.2%. The diff coverage is 55.8%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #606     +/-   ##
========================================
+ Coverage    58.7%   59.0%   +0.2%     
========================================
  Files         128     123      -5     
  Lines        9195    8716    -479     
========================================
- Hits         5400    5143    -257     
+ Misses       3795    3573    -222     
Impacted Files Coverage Δ
labgrid/remote/client.py 47.1% <0.0%> (+0.2%) :arrow_up:
labgrid/resource/udev.py 47.8% <66.6%> (+0.5%) :arrow_up:
labgrid/remote/exporter.py 63.9% <71.4%> (+1.6%) :arrow_up:
labgrid/resource/remote.py 82.0% <75.0%> (-0.1%) :arrow_down:
labgrid/driver/powerdriver.py 67.8% <100.0%> (ø)
labgrid/resource/__init__.py 100.0% <100.0%> (ø)
labgrid/target.py 89.1% <0.0%> (-3.4%) :arrow_down:
labgrid/factory.py 84.6% <0.0%> (-1.7%) :arrow_down:
labgrid/config.py 80.2% <0.0%> (-1.6%) :arrow_down:
labgrid/driver/flashromdriver.py 63.8% <0.0%> (-1.4%) :arrow_down:
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 02d5b1b...cd1b4c9. Read the comment docs.

codecov[bot] avatar May 19 '20 22:05 codecov[bot]

In the medium term, I'd like to replace labgrid's powerdrivers with pdudaemon, which also has support for YKUSH (at least one of the versions). @mattface indicated that the approach to simplify it and support blocking requests (https://github.com/pdudaemon/pdudaemon/pull/78) can continue.

jluebbe avatar May 20 '20 06:05 jluebbe

In the medium term, I'd like to replace labgrid's powerdrivers with pdudaemon, which also has support for YKUSH (at least one of the versions). @mattface indicated that the approach to simplify it and support blocking requests (pdudaemon/pdudaemon#78) can continue.

Does this mean you want me to abandon this pull request? I'd prefer to go on with this to get something working in the short term, then we can move that to pdudaemon in the future.

mbgg avatar May 20 '20 09:05 mbgg

Does this mean you want me to abandon this pull request? I'd prefer to go on with this to get something working in the short term, then we can move that to pdudaemon in the future.

Besides the missing maintenance, what are the issues you have with https://github.com/Yepkit/pykush/? Just the missing remote support? I took a short look at the ykushcmd implementation and it seemed to have a lot of complexity for something that should be relatively simple. ;)

If you can solve the inconsistency between matching on serial number vs. matching on the bus topology, I'd merge the PR, as it would allow us to access them remotely. I'd suspect though, that finishing that pdudaemon PR may be easier to do.

jluebbe avatar May 20 '20 12:05 jluebbe

Does this mean you want me to abandon this pull request? I'd prefer to go on with this to get something working in the short term, then we can move that to pdudaemon in the future.

Besides the missing maintenance, what are the issues you have with https://github.com/Yepkit/pykush/? Just the missing remote support? I took a short look at the ykushcmd implementation and it seemed to have a lot of complexity for something that should be relatively simple. ;)

The problem with pykush is that the pip version has changed and doesn't work with the labgrid implementation anymore. So you would need to install pykush from https://github.com/Yepkit/pykush instead from pip.

If you can solve the inconsistency between matching on serial number vs. matching on the bus topology, I'd merge the PR, as it would allow us to access them remotely. I'd suspect though, that finishing that pdudaemon PR may be easier to do.

Could we use ID_SERIAL_SHORT from udevadm info for matching in udev?

It shows me: E: ID_SERIAL_SHORT=YKC6071

mbgg avatar May 20 '20 17:05 mbgg

Does this mean you want me to abandon this pull request? I'd prefer to go on with this to get something working in the short term, then we can move that to pdudaemon in the future.

Besides the missing maintenance, what are the issues you have with https://github.com/Yepkit/pykush/? Just the missing remote support? I took a short look at the ykushcmd implementation and it seemed to have a lot of complexity for something that should be relatively simple. ;)

The problem with pykush is that the pip version has changed and doesn't work with the labgrid implementation anymore. So you would need to install pykush from https://github.com/Yepkit/pykush instead from pip.

I've only ever tried it with the version from git, which is also referred to in the source. We could add a git entry for it in a requirements.txt.

If you can solve the inconsistency between matching on serial number vs. matching on the bus topology, I'd merge the PR, as it would allow us to access them remotely. I'd suspect though, that finishing that pdudaemon PR may be easier to do.

Could we use ID_SERIAL_SHORT from udevadm info for matching in udev?

It shows me: E: ID_SERIAL_SHORT=YKC6071

I don't have a ykush available for testing. Is that the same serial as used with the command line tool?

jluebbe avatar May 21 '20 09:05 jluebbe

Does this mean you want me to abandon this pull request? I'd prefer to go on with this to get something working in the short term, then we can move that to pdudaemon in the future.

Besides the missing maintenance, what are the issues you have with https://github.com/Yepkit/pykush/? Just the missing remote support? I took a short look at the ykushcmd implementation and it seemed to have a lot of complexity for something that should be relatively simple. ;)

The problem with pykush is that the pip version has changed and doesn't work with the labgrid implementation anymore. So you would need to install pykush from https://github.com/Yepkit/pykush instead from pip.

I've only ever tried it with the version from git, which is also referred to in the source. We could add a git entry for it in a requirements.txt.

That would work as well. I can find out how to do that.

If you can solve the inconsistency between matching on serial number vs. matching on the bus topology, I'd merge the PR, as it would allow us to access them remotely. I'd suspect though, that finishing that pdudaemon PR may be easier to do.

Could we use ID_SERIAL_SHORT from udevadm info for matching in udev? It shows me: E: ID_SERIAL_SHORT=YKC6071

I don't have a ykush available for testing. Is that the same serial as used with the command line tool?

Yes:

# ykushcmd -l

Attached YKUSH Boards

---------------------
YKUSH device found with Serial Number: YKC6071
  Manufacturer: Yepkit Lda.
  Product:      YKUSH

I'm not sure how I'll have to implement this. Using the existing ID_VENDOR_ID and ID_MODEL_ID and add ID_SERIAL_SHORT or just use ID_SERIAL_SHORT?

mbgg avatar May 22 '20 14:05 mbgg

Ok the pull request now add the pykush dependency to the requierements file and matches against the serial number of the YKUSH.

@jluebbe please review again :)

mbgg avatar May 28 '20 20:05 mbgg

I have pushed a commit onto your branch which should fix the travis failure.

Emantor avatar May 29 '20 06:05 Emantor

I have pushed a commit onto your branch which should fix the travis failure.

Thanks! This fixed the issue in travis but not yet in docker/build.sh

mbgg avatar May 29 '20 09:05 mbgg

@jluebbe any comments on this? It's been a while since I pushed to code for review :)

mbgg avatar Oct 01 '20 10:10 mbgg

@mbgg I guess most of the issues this PR tried to solve are now solved by #1143. Is there something left? If not, can we close this?

Bastian-Krause avatar Jun 22 '23 11:06 Bastian-Krause