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

fix(protocols/modbus): fix write requests for coils always set to false (#710) #711

Merged
merged 2 commits into from Dec 29, 2022

Conversation

SteinOv
Copy link
Contributor

@SteinOv SteinOv commented Dec 23, 2022

see issue-710

@chrisdutz
Copy link
Contributor

I think in this case it would then work for the coils, but break for the registers, as these are 16 bit units.

I think a better solution would be to have either a dedicated DataIo for coil data (However this would have very few cases) or to not use the DataIo component for parsing the coil data, but to implement it manually (which shouldn't be much work) for this case (One could argue, that reading 8 coils and having that interpreted as a signed 8-bit integer would no longer be supported by that, but that's probably something I would be able to live with)

Copy link
Contributor

@chrisdutz chrisdutz left a comment

Choose a reason for hiding this comment

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

This change will definitely break boolean support for registers.

@SteinOv
Copy link
Contributor Author

SteinOv commented Dec 23, 2022

What kind of solution did you have in mind and why would it break the functionality you're describing?
I thought maybe adding an if statement to the fromPlcValue() method in ModbusProtocolLogic: if (tag instanceof ModbusTagCoil && plcValue instanceof PlcBOOL) and then manually parsing the value there.
What do you think?

@chrisdutz
Copy link
Contributor

Oh ... thought I had mentioned this on the main PR comment:
Reason is that for Registers it sort of expects 16 bit ... if we reduce this to 8 again, we will only be able to detect the numeric value of 8 as a true.
I think the ideal solution would be to have:

  • A separate data io for coils
  • Extend the existing data io with an additional property, if this is a coil or a register and to have various options for dealing with it
  • Simply manually implement the parsing of coils

My prefered option would be the 3rd ... manually implementing the parsing of coils.

@SteinOv
Copy link
Contributor Author

SteinOv commented Dec 23, 2022

Sorry, I wasn't clear. My comment was a response to your first comment. You said that manually implementing the parsing the coil data would break being able to interpret 8 coils as an signed 8-bit integer. I was wondering what kind of solution for manually implementing the parsing of the coil data you had in mind and how this would break that functionality.
I also thought about it myself and suggested a way to manually implement the parsing, see my previous comment.

…ally implemented parsing of coil boolean instead (apache#710)
@SteinOv
Copy link
Contributor Author

SteinOv commented Dec 27, 2022

See my latest commit for what I had in mind.

@chrisdutz
Copy link
Contributor

Hi,

Yeah ... this should be a good solution. And it's way less complicated ... but happy you at least could have a look at the general concept of our code-generation ;-)

Chris

@chrisdutz chrisdutz merged commit c9a1938 into apache:develop Dec 29, 2022
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.

None yet

2 participants