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

Add devantech dS378 ethernet relay agent #694

Merged
merged 11 commits into from
Jan 22, 2025
Merged

Conversation

dixilo
Copy link
Contributor

@dixilo dixilo commented Jun 14, 2024

Description

This pull request adds a new agent to operate devantech's dS378 ethernet relay.

Motivation and Context

Since dS378 is used in the electronics of the stimulator, the agent should be included in this repository.

How Has This Been Tested?

Test was done using the actual dS378 device.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@dixilo dixilo requested a review from BrianJKoopman June 14, 2024 02:08
@BrianJKoopman BrianJKoopman added the new agent New OCS agent needs to be created label Oct 3, 2024
Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! And sorry its taken so long to get to reviewing. This looks really good overall, just a couple of minor comments, mostly related to documentation. I also made a comment about robustness to network outages, but again that can be done later if needed.

Please make a merge commit to merge in the latest from main while you're working on these.

docs/agents/devantech_dS378.rst Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/drivers.py Outdated Show resolved Hide resolved
docs/agents/devantech_dS378.rst Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Show resolved Hide resolved
socs/agents/devantech_dS378/drivers.py Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/drivers.py Show resolved Hide resolved
@dixilo
Copy link
Contributor Author

dixilo commented Jan 9, 2025

Thanks for your detailed review!
I modified the codes according to your review comments.

@BrianJKoopman BrianJKoopman self-requested a review January 9, 2025 14:23
This isn't needed, since the params decorator prevents this ever being None.
Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for addressing all the comments, and for your detailed responses! A few more small suggestions.

It's great to see use of the TCPInterface class too, glad that was straightforward. Just to confirm, the agent now can gracefully recover itself from a network interruption, say if you disconnected the network cable to the dS378?

I'm going to push a couple of small changes as well that I figured would just be quicker to do while I was re-reviewing. Please be sure to pull those before continuing with the suggested changes. Thanks again!

socs/agents/devantech_dS378/agent.py Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/agent.py Outdated Show resolved Hide resolved
socs/agents/devantech_dS378/drivers.py Show resolved Hide resolved
@dixilo
Copy link
Contributor Author

dixilo commented Jan 20, 2025

Thanks for the review and the comments!
I modified the codes according to your suggestions.

@dixilo dixilo requested a review from BrianJKoopman January 20, 2025 09:50
Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

Thanks for implementing the suggestions, looks great!

@BrianJKoopman BrianJKoopman merged commit 265c0fc into main Jan 22, 2025
7 checks passed
@BrianJKoopman BrianJKoopman deleted the jsuzuki/stimulator/dS378 branch January 22, 2025 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new agent New OCS agent needs to be created
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants