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

WIP: Implements reading ability for PLC-5 #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

WIP: Implements reading ability for PLC-5 #33

wants to merge 1 commit into from

Conversation

gfcittolin
Copy link
Contributor

This branch implements the ability to read (and maybe write in the future) data from/to PLC-5 and PLC-5/250 series of PLCs. It does so by implementing the PLC-5 command set, and allowing it to be chosen at constructor time, enabling further command sets to be developed. It defaults although to the existing SLC command set, so backwards compatibility is maintained.

This is still a Work In Progress: It has been only tested with a couple of tags against a single PLC, so more testing is very welcome in case anybody has access to such PLCs.

@plcpeople plcpeople marked this pull request as ready for review January 11, 2020 01:41
Copy link
Owner

@plcpeople plcpeople left a comment

Choose a reason for hiding this comment

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

I think I marked this PR "Ready for review" and did not mean to do that. I haven't used draft PRs much.

The code looks good to me and it's good to see this support integrated. I don't have access to a PLC-5 so I'll have to take your word that it works well but I don't see how it could cause problems with existing working applications.

Do you want this published as is or do you want to do anything else? Before publishing, I'd like to add something to the documentation like:
This requires a minimum PLC-5E revisions that supports EIP/PCCC - at least:

  • Series C, Revision N.1
  • Series D, Revision E.1
  • Series E, Revision D.1

(Assuming you agree with this info, I found it quite quickly)

I can add this to the documentation if you want.

Do the response packets from the PLC-5 look identical, at least in your testing?

@gfcittolin
Copy link
Contributor Author

My idea with the draft PR was to inform about the development, and allow anyone that has access to a PLC-5 (and is watching this repo) to be able to test on their systems and report any possible issue. I wouldn't merge it yet, as I've tested with a single CPU and just a couple of tags. The docs are also not much clear about the meaning of some fields, another reason to test it better before merging.

It would also be nice to implement the writing command as well before merging, or at least fail gracefully instead of just sending the same wrong command, the current behavior.

Do the response packets from the PLC-5 look identical, at least in your testing?

Luckily yes, at least according to the docs and for the couple tags I've tested. Again, more testing could expose some edge cases I'm not aware of.

Regarding the docs, I agree we need to update them, adding an example of the new option and the supported PLCs. As I'll still do some changes to the code, I can change the docs too.

Thanks for your time reviewing the code!

@gfcittolin gfcittolin changed the title Implements reading ability for PLC-5 WIP: Implements reading ability for PLC-5 Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants