Skip to content

driver: add modbus rtu driver#806

Merged
jluebbe merged 1 commit intolabgrid-project:masterfrom
b2vn:feature/add-modbusrtu
Dec 10, 2021
Merged

driver: add modbus rtu driver#806
jluebbe merged 1 commit intolabgrid-project:masterfrom
b2vn:feature/add-modbusrtu

Conversation

@b2vn
Copy link
Copy Markdown
Contributor

@b2vn b2vn commented Jul 26, 2021 •
edited by jluebbe
Loading

Description

Added a Modbus RTU driver to allow for labgrid tests to control modbus devices.

Checklist

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

@b2vn b2vn force-pushed the feature/add-modbusrtu branch from 01a5919 to 8cad450 Compare July 26, 2021 09:42
@b2vn
Copy link
Copy Markdown
Contributor Author

b2vn commented Jul 26, 2021

@Emantor, @Bastian-Krause would you take a look at this?

Emantor
Emantor reviewed Jul 26, 2021
View reviewed changes
Comment thread labgrid/resource/modbusrtu.py Outdated
"""
port = attr.ib(validator=attr.validators.instance_of(str))
address = attr.ib(validator=attr.validators.instance_of(int))
speed = attr.ib(default=115200,
Copy link
Copy Markdown
Member

@Emantor Emantor Jul 26, 2021

Choose a reason for hiding this comment

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

The SerialPort baseclass already has port & speed, why is this redefined here?

Comment thread labgrid/driver/modbusrtudriver.py
Comment thread modbusrtu-requirements.txt
Comment thread doc/configuration.rst Outdated

ModbusRTU
+++++++++
Descriped the resource required to use the Modbus RTU driver
Copy link
Copy Markdown
Member

@Emantor Emantor Jul 26, 2021

Choose a reason for hiding this comment

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

A bit more explanation would be helpful, maybe a link to a Modbus RTU protocol description. Is Modbus RTU always used over serial interfaces?

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 26, 2021 •
edited
Loading

Codecov Report

Merging #806 (8428a0b) into master (dc1edb0) will increase coverage by 0.1%.
The diff coverage is 79.2%.

❗ Current head 8428a0b differs from pull request most recent head 2cfbd82. Consider uploading reports for the commit 2cfbd82 to get more accurate results
Impacted file tree graph

@@           Coverage Diff            @@
##           master    #806     +/-   ##
========================================
+ Coverage    56.7%   56.9%   +0.1%     
========================================
  Files         146     147      +1     
  Lines       10915   10942     +27     
========================================
+ Hits         6193    6228     +35     
+ Misses       4722    4714      -8
Impacted Files Coverage Δ
labgrid/driver/modbusrtudriver.py 75.0% <75.0%> (ø)
labgrid/resource/modbusrtu.py 86.6% <86.6%> (ø)
labgrid/driver/__init__.py 100.0% <100.0%> (ø)
labgrid/resource/__init__.py 100.0% <100.0%> (ø)
labgrid/driver/power/tplink.py
labgrid/target.py 93.2% <0.0%> (+0.8%) ⬆️

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 dc1edb0...2cfbd82. Read the comment docs.

@b2vn b2vn force-pushed the feature/add-modbusrtu branch 4 times, most recently from be635f7 to 565c401 Compare July 30, 2021 20:49
@b2vn
Copy link
Copy Markdown
Contributor Author

b2vn commented Aug 4, 2021

@Emantor Could you have another look at this?

Emantor
Emantor previously approved these changes Sep 15, 2021
View reviewed changes
@Emantor Emantor requested a review from jluebbe September 15, 2021 14:21
@jluebbe jluebbe force-pushed the feature/add-modbusrtu branch from 565c401 to 52ab8da Compare September 16, 2021 10:51
jluebbe
jluebbe previously approved these changes Sep 16, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jluebbe jluebbe 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.

I've fixed some typos in the docs and think we can merge this as is.

@b2vn, if you have some time, you could add @Driver.check_active to the driver functions to avoid failing only when trying to access the instrument property.

Also, the checks in the resource __attrs_post_init__ should be better handled via attrs.

@Emantor Emantor added enhancement needs rebase Needs a rebase onto the master branch, maintainter could probably not push to submitter branch. labels Oct 6, 2021
@Emantor Emantor self-assigned this Nov 26, 2021
@Emantor Emantor force-pushed the feature/add-modbusrtu branch from 52ab8da to 8428a0b Compare November 26, 2021 08:00
Emantor
Emantor previously approved these changes Nov 26, 2021
View reviewed changes
@b2vn @Emantor
driver: add modbus rtu driver
2cfbd82 
Signed-off-by: Nikolaj Rahbek <nikolaj@b2vn.org>
@Emantor Emantor force-pushed the feature/add-modbusrtu branch from 8428a0b to 2cfbd82 Compare November 26, 2021 14:11
@jluebbe
Copy link
Copy Markdown
Member

jluebbe commented Dec 10, 2021

Although the GitHub UI doesn't show it, the CI run was successful: https://github.com/labgrid-project/labgrid/actions/runs/1506655629

@jluebbe jluebbe merged commit 9afa321 into labgrid-project:master Dec 10, 2021
@jluebbe jluebbe mentioned this pull request Jul 17, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Emantor Emantor Emantor approved these changes

@jluebbe jluebbe jluebbe approved these changes

Assignees

@Emantor Emantor

Labels

enhancement needs rebase Needs a rebase onto the master branch, maintainter could probably not push to submitter branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@b2vn @jluebbe @Emantor