Skip to content

driver/powerdriver: Add SerialPortPowerdriver #192

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.

Sign up for GitHub

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

Jump to bottom
Merged
jluebbe merged 2 commits into from
Mar 19, 2018
Merged

driver/powerdriver: Add SerialPortPowerdriver #192

jluebbe merged 2 commits into from
Mar 19, 2018

Conversation

@SmithChart
Copy link
Member

@SmithChart SmithChart commented Feb 12, 2018 •
edited
Loading

Adding a powerdriver that controls an external relais board connected to
the control signals of a USB-UART-dongle.

This is a poor-mans solution for a power switch that can be used for
desktops with only a single place.

@SmithChart SmithChart mentioned this pull request Feb 12, 2018
Merged
@SmithChart SmithChart force-pushed the SerialPortPowerdriver branch from 33973d8 to 3bfe23e Compare February 13, 2018 20:27
Emantor
Emantor reviewed Feb 15, 2018
View reviewed changes
Copy link
Member

@Emantor Emantor left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Is there a specific reason why simultanuous use for communication will not work?

Review comments based on the assumption that there is none. Thinking further about this it might be even easier and more straightforward to extend the SerialDriver with the PowerProtocol and handle everything there.

labgrid/driver/powerdriver.py Outdated
Usable signals are DTR and RTS.
"""

port = attr.ib(validator=attr.validators.instance_of(str))
Copy link
Member

@Emantor Emantor Feb 15, 2018

Choose a reason for hiding this comment

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

Please bind to a SerialPort Resource instead of taking the raw /dev/path here.

labgrid/driver/powerdriver.py Outdated
"""

port = attr.ib(validator=attr.validators.instance_of(str))
signal= attr.ib(validator=attr.validators.instance_of(str))
Copy link
Member

@Emantor Emantor Feb 15, 2018 •
edited by jluebbe
Loading

Choose a reason for hiding this comment

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

IMO extending the existing resources with an optional power_signal is more straightforward.

@SmithChart
Copy link
Member Author

SmithChart commented Feb 15, 2018 •
edited by jluebbe
Loading

I am not sure about the specific reason: I tested using the same port for both - and it did behave strange. I did not invest time to investigate the behavior.

I will have a look at integrating a PowerProtocoll into the SerialPortDriver.

@jluebbe
Copy link
Member

jluebbe commented Feb 15, 2018

@SmithChart We shouldn't make the normal SerialDriver implement the PowerProtocol, as this would require using named drivers for the common case of a separate power driver.

You could make this a a ConsolePIODriver, which binds to a ConsoleProtocol to export a control line as a PIO, then use the existing DigitalOutputPowerDriver for power control. We'd need to add a method to set/get control lines to ConsoleProtocol for this to work though. What do you think?

@SmithChart
Copy link
Member Author

SmithChart commented Feb 16, 2018

You could make this a a ConsolePIODriver, which binds to a ConsoleProtocol to export a control line as a PIO, then use the existing DigitalOutputPowerDriver for power control. We'd need to add a method to set/get control lines to ConsoleProtocol for this to work though. What do you think?

I am not sure if it is a good idea to actually dig this feature so deep into the labgrid infrastructure.
Binding the driver to a serial port as suggested by @Emantor makes total sense to me.

Adding features to the ConsolePrrotocol just to support this featrue seems to softens the rather sharp outline of the architecture. This feature is meant to help a beginner to build a simple setup to start with labgrid.

Maybe it would be a better idea to alter the driver to a DigitalOutput driver and use the DigitalOutputPowerDriver on top of that.

@SmithChart SmithChart force-pushed the SerialPortPowerdriver branch 2 times, most recently from 14e81dc to afaffb1 Compare February 23, 2018 18:18
@SmithChart
Copy link
Member Author

SmithChart commented Feb 23, 2018

I had discusses this change with @jluebbe face-to-face.

I splitted this driver into two parts. A DigitalOutputDriver and a DigitalOutputPowerSwitch.

For more details see the commit messages.

@SmithChart SmithChart force-pushed the SerialPortPowerdriver branch from afaffb1 to 5afd410 Compare February 23, 2018 18:32
@SmithChart SmithChart force-pushed the SerialPortPowerdriver branch from 5afd410 to aaddc09 Compare March 3, 2018 15:43
@SmithChart
Copy link
Member Author

SmithChart commented Mar 3, 2018

Ping

I rebased my commits. Is there anything else missing?

@SmithChart
Copy link
Member Author

SmithChart commented Mar 16, 2018

Ping

Still waiting for feedback here.

@jluebbe jluebbe force-pushed the SerialPortPowerdriver branch 2 times, most recently from 766209c to 1ce1f4e Compare March 19, 2018 11:54
@jluebbe jluebbe self-assigned this Mar 19, 2018
@SmithChart @jluebbe
driver: add SerialDigitalOutoutDriver
1c3fa85 
The SerialDigitalOutoutDriver sits on top of a SerialDriver and uses the
underlying pyserial object to control a flow control line of a serial
port as if it were general-purpose outputs.

With this driver a free serial port can be used to control
a general purpose output for eg. choosing of a boot-target.

Signed-off-by: Chris Fiege <chris@tinyhost.de>
@SmithChart @jluebbe
driver/powerdriver: add DigitalOutputPowerDriver
82e94f2 
This driver sits on top of a DigitalOutput and provides a PowerProtocol.

An old version of this driver combined resetting a DUT via a GPIO and
power-cycling via PowerSwitch and  was removed when adding the
ResetProtocol. This new driver only uses a switch controlled via the
DigitalOutputProtocol.

Signed-off-by: Chris Fiege <chris@tinyhost.de>
@jluebbe jluebbe force-pushed the SerialPortPowerdriver branch from 1ce1f4e to 82e94f2 Compare March 19, 2018 12:03
@codecov-io
Copy link

codecov-io commented Mar 19, 2018 •
edited
Loading

Codecov Report

Merging #192 into master will increase coverage by 0.1%.
The diff coverage is 66.1%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #192     +/-   ##
========================================
+ Coverage    54.8%   54.9%   +0.1%     
========================================
  Files          97      98      +1     
  Lines        5424    5482     +58     
========================================
+ Hits         2975    3013     +38     
- Misses       2449    2469     +20
Impacted Files Coverage Δ
labgrid/driver/__init__.py 100% <100%> (ø) ⬆️
labgrid/driver/serialdigitaloutput.py 63.8% <63.8%> (ø)
labgrid/driver/powerdriver.py 79.8% <66.6%> (-2.9%) ⬇️

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 9404b94...82e94f2. Read the comment docs.

@jluebbe jluebbe merged commit 7e07b44 into labgrid-project:master Mar 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Emantor Emantor Emantor left review comments

@jluebbe jluebbe jluebbe approved these changes

Assignees

@jluebbe jluebbe

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@SmithChart @jluebbe @codecov-io @Emantor