Make Pylogix Port configurable

caprinux commented 1 year ago

Short description of change

Prior to this PR, pylogix port is hardcoded to 44818. While this is understandable, since the port for the protocol defaults to 44818, we can make it default to 44818 but also configurable at the same time.

NOTE: not tested, documentation not added yet

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to change)
[ ] I have read the docs/CONTRIBUTING.md document.
[ ] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have read tests/README.md.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

What is the change?

add Port paramter in PLC class that defaults to 44818, but can be configured

What does it fix/add?

make pylogix port configurable

Test Configuration

PLC Model
PLC Firmware
pylogix version
python version
OS type and version

TheFern2 commented 1 year ago

Thanks for the PR, can you please give use cases where this change would be beneficial? Where in the eip communication pipeline are you changing the port to something else other than 44818?

As a side note, this is a breaking change since you're adding a parameter in the middle, so, if users have not used named parameters, and have slot number, the port would be taking its place.

i.e:

# some code that would break
comm = PLC("192.168.1.5", 2)  # <- slot is now taken as port, if you were to update pylogix with this PR.

dmroeder commented 1 year ago

We definitely don't want to have a compatibility issue with this. I don't think communications will take place on any other port, so I'm not sure what we are accomplishing by making it configurable. Other than giving people an nob to turn that shouldn't be turned.

evaldes2015 commented 1 year ago

If you want to talk to the the PLC over a port forward it might be useful to have configurable ports. EIP is not secure. If you want to encrypt it, you could set up a gateway machine that you have to connect to via SSH. You then would connect to the PLC via a port forward. A single gateway could forward to multiple PLCs, which would require multiple listening ports.

kyle-github commented 1 year ago

Er, yeah. I ended up supporting configurable ports as well. Don't remember all the arguments. They were rare cases, but very compelling ones.

TheFern2 commented 1 year ago

I can see port fwd being a good use case. I'll say let's adopt the change with minimal impact to existing users. Move port parameter to the end of PLC class or you could just have it as a variable and change it directly since it is rare that someone will want to change it probably the better option of the two.

comm = PLC("192.168.1.5", 2)
comm.Port = 12312

On lgx_comm.py, I don't think discover needs a port parameter since you already have self.parent.Port available.

dmroeder commented 1 year ago

I can see port fwd being a good use case. I'll say let's adopt the change with minimal impact to existing users. Move port parameter to the end of PLC class or you could just have it as a variable and change it directly since it is rare that someone will want to change it probably the better option of the two.
comm = PLC("192.168.1.5", 2)
comm.Port = 12312
On lgx_comm.py, I don't think discover needs a port parameter since you already have self.parent.Port available.

I'm on board with these suggestions.

caprinux commented 1 year ago

Hi all, thank you for your inputs! I made this PR due to my encounters of having to interact with PLCs via EIP over non-conventional port (44818), likely due to port forwarding.

Just to share for fun, when I was searching for existing projects/tools that allowed me to easily interact with PLCs over EIP on a configurable port, chatGPT brought me here and suggested that I could configure the port as such:

comm.Port = 1234

However, this obviously didn't work and after I debugged and figured out why, I figured that I should make a PR.

I've since made the necessary changes so as to not break the order of parameters. I also personally feel that the small change should have little to no downside for the project, apart from improving QoL for users.

Do provide more feedback if necessary. Thank you! :)

dmroeder / pylogix