pymodbus-dev / pymodbus

A full modbus protocol written in python
Other
2.16k stars 891 forks source link

Generic Encapsulated Interface Transport 0x2B message handler #2059

Closed dnssoftware closed 3 months ago

dnssoftware commented 4 months ago

Is there an interest to add generic 0x2B handler? I've been testing this for few months now

janiversen commented 4 months ago

Please rebase the pull request to the newest dev. Repl is no longer in this repo.

Please also add a couple of tests, to ensure it works.

Once done, I will make a review, but it looked quite ok.

dnssoftware commented 4 months ago

Please rebase the pull request to the newest dev. Repl is no longer in this repo.

Please also add a couple of tests, to ensure it works.

Once done, I will make a review, but it looked quite ok.

Many thanks. Not a problem. I've rebased and next I have to organise a test.

Dean

janiversen commented 4 months ago

What happened?

dnssoftware commented 4 months ago

What happened?

Hi Jan,

I’m trying to work out what kind of test I have to write. I was looking at ReadDeviceInformation but couldn’t see a specialised test to use as reference.

Familiarising myself with the code at the moment.

janiversen commented 4 months ago

Ahhh, then no need to close it.

Please look at

test/sub_messages/test_mei_messages.py

That is the place to add a test.

examples/message_parser.py

uses mei and others to pass all types of messages.

It seems your code was based on a rather old version of pymodbus and need a bit of update:

run ./check_ci.sh gives you the CI problem and automatically repairs things like spaces.

dnssoftware commented 4 months ago

Ahhh, then no need to close it.

Please look at

test/sub_messages/test_mei_messages.py

That is the place to add a test.

examples/message_parser.py

uses mei and others to pass all types of messages.

It seems your code was based on a rather old version of pymodbus and need a bit of update:

run ./check_ci.sh gives you the CI problem and automatically repairs things like spaces.

Thanks for the pointers.

Yes, I think I first forked the project back in October, wrote what I needed and been using it since internally. As it seemed quite useful, considering porting a c++ project to python and I realised it would be easier if I no longer maintain a private version but if what I use is useful enough to be incorporated into the main.

Bear with me, I'll will fill in the missing bits as time permits.

janiversen commented 4 months ago

Take your time, we are all volunteers.

Keeping the PR open, means I and others are aware of your ongoing work, and thus can advice you if we see conflicting work.

As long as your amendments do not break the standard, we are very open to changes...and yes it is surely better to have the changes in the main repo, since it then gets automatically updated.

Have a nice day.

janiversen commented 3 months ago

Isnt it about time, to get this ready to be merged ?

dnssoftware commented 3 months ago

Isnt it about time, to get this ready to be merged ?

Hi Jan,

Sorry, no. My mods aren't good enough at this stage and I haven't had a chance to pursue further at the moment.

janiversen commented 3 months ago

I did a bit of testing this morning.

First I cleaned your PR of a lot of unrelated changes (like removing "." at the end of several methods Secondly I try to understand how this could have been working for month (I can't even get it to work for 1 minute)

It seems the way you integrated the new function into factory.py, broke e.g. read_device_information.

I think a generic handler is difficult to implement and difficult to use at API level, it is much better to add missing sub_function codes.

I have published my branch, so you can see the changes I have made to your PR.

dnssoftware commented 3 months ago

Lets leave it for better times