feat(plc4go): Implementing the correct reading of BOOL types #545
Conversation
|
Guess I'll have to fully review this next week (Currently travelling) ... also would I like to continue the discussion on the list on if my proposal even makes sense ... cause I think we never finished that discussion. I probably should have marked that document as Work In Progress. |
|
Hi, Could you please update your PR? I'd love to finialize it and merge it as soon as possible. Chris |
|
Hi Chris, welcome back. |
|
@chrisdutz I have updated it, and have a commit(17d7f76) about it, please review it when you have time. Jinlin |
| @@ -31,7 +31,7 @@ func main() { | |||
| drivers.RegisterModbusTcpDriver(driverManager) | |||
|
|
|||
| // Get a connection to a remote PLC | |||
| crc := driverManager.GetConnection("modbus-tcp://192.168.23.30") | |||
| crc := driverManager.GetConnection("modbus-tcp://192.168.10.180") | |||
There was a problem hiding this comment.
We should probably externalize this because every one is probably going to have the device at a different IP
| value, _valueErr := readBuffer.ReadBit("value") | ||
| if _valueErr != nil { | ||
| return nil, errors.Wrap(_valueErr, "Error parsing 'value' field") | ||
| _numberOfValues := uint16(math.Ceil(float64((offset + numberOfValues)) / float64(16))) |
There was a problem hiding this comment.
In this branch "numberOfValues" is always 1
| @@ -24,30 +24,48 @@ import ( | |||
| "github.com/apache/plc4x/plc4go/spi/utils" | |||
| "github.com/apache/plc4x/plc4go/spi/values" | |||
| "github.com/pkg/errors" | |||
| "math" | |||
| ) | |||
|
|
|||
| // Code generated by code-generation. DO NOT EDIT. | |||
There was a problem hiding this comment.
I hope you read this line and didn't manually edit this file ... because the next time you'll run the full maven build these changes are going to be replaced with the old values.
| @@ -40,7 +40,7 @@ func (m ModbusParserHelper) Parse(typeName string, arguments []string, io utils. | |||
| if err != nil { | |||
| return nil, errors.Wrap(err, "Error parsing") | |||
| } | |||
| return model.DataItemParse(io, dataType, numberOfValues) | |||
| return model.DataItemParse(io, dataType, numberOfValues, 0) | |||
There was a problem hiding this comment.
This is a generated file and will be replaced by an updated version, effectively reverting your changes the next time "mvn install" is executed in the parent.
| @@ -51,7 +51,7 @@ func (m ModbusXmlParserHelper) Parse(typeName string, xmlString string, parserAr | |||
| return nil, err | |||
| } | |||
| numberOfValues := uint16(parsedUint1) | |||
| return model.DataItemParse(utils.NewXmlReadBuffer(strings.NewReader(xmlString)), dataType, numberOfValues) | |||
| return model.DataItemParse(utils.NewXmlReadBuffer(strings.NewReader(xmlString)), dataType, numberOfValues, 0) | |||
There was a problem hiding this comment.
This is a generated file and will be replaced by an updated version, effectively reverting your changes the next time "mvn install" is executed in the parent.
|
In general, the changes look good, however you manually seem to have edited generated code, so the changes will get lost the next time the maven build is executed. |
Hi Chris, Thank you very much for reviewing the code. I have a revert commit(8a793e2) of this commit(17d7f76) after Ben remind me that the Golang build failed after my push, sorry for that. The reason the Golang build failed is just what you said I edited generated file directly. Jinlin |
|
Always happy to help educate :-) |
|
@chrisdutz @hongjinlin as far as I can see this PR was ready to be merged? Seems then only a merge/rebase might be necessary. |
|
No objections to merging |
|
@hongjinlin can you merge main into this branch or rebase? |
Implemented according to Cleanup of how we handle all the bit-related fields
Thanks for reviewing the code.