pymodbus icon indicating copy to clipboard operation
pymodbus copied to clipboard

Generic Encapsulated Interface Transport 0x2B message handler

Open dnssoftware opened this issue 1 year ago • 7 comments

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

dnssoftware avatar Feb 25 '24 13:02 dnssoftware

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.

janiversen avatar Feb 25 '24 13:02 janiversen

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

dnssoftware avatar Feb 25 '24 22:02 dnssoftware

What happened?

janiversen avatar Feb 26 '24 07:02 janiversen

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.

dnssoftware avatar Feb 26 '24 07:02 dnssoftware

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.

janiversen avatar Feb 26 '24 07:02 janiversen

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.

dnssoftware avatar Feb 26 '24 08:02 dnssoftware

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 avatar Feb 26 '24 10:02 janiversen

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

janiversen avatar Mar 24 '24 12:03 janiversen

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.

dnssoftware avatar Mar 25 '24 08:03 dnssoftware

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.

janiversen avatar Mar 27 '24 13:03 janiversen

Lets leave it for better times

dnssoftware avatar Mar 27 '24 21:03 dnssoftware