You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by GitBox <gi...@apache.org> on 2022/10/13 06:52:15 UTC
[GitHub] [plc4x] hongjinlin opened a new pull request, #545: feat(plc4go): Implementing the correct reading of BOOL types
hongjinlin opened a new pull request, #545:
URL: https://github.com/apache/plc4x/pull/545
Implemented according to [Cleanup of how we handle all the bit-related fields](https://cwiki.apache.org/confluence/display/PLC4X/Cleanup+of+how+we+handle+all+the+bit-related+fields)
Thanks for reviewing the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1374593134
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #545:
URL: https://github.com/apache/plc4x/pull/545#discussion_r1064040985
##########
plc4go/protocols/modbus/readwrite/model/DataItem.go:
##########
@@ -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.
Review 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.
##########
plc4go/protocols/modbus/readwrite/ParserHelper.go:
##########
@@ -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)
Review 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.
##########
plc4go/protocols/modbus/readwrite/XmlParserHelper.go:
##########
@@ -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)
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1277625494
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.
Thing is ... I think the general notation used in many places would have more been something like BOOL[5..10] or so ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1375345033
Always happy to help educate :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4go): Implementing the correct reading of BOOL types
Posted by "sruehl (via GitHub)" <gi...@apache.org>.
sruehl commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1733858802
@hongjinlin can you merge main into this branch or rebase?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] hongjinlin commented on pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
hongjinlin commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1367222511
Hi Chris, welcome back.
Did you mean to let me update PR for being merged smoothly or update for other things?
But whatever update is needed will be a few days away, as I'm on day two of Covid-19.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] hongjinlin commented on pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
hongjinlin commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1375071236
> 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(https://github.com/apache/plc4x/commit/8a793e26d8b24060ee657d7ca9e6114d89c724a1) of this commit(https://github.com/apache/plc4x/commit/17d7f765c670f86c3fd110f010a3faafe8ee1c5a) 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.
But don’t worry I will be familiar with the code generation and have a PR for that as soon as possible.
Jinlin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #545:
URL: https://github.com/apache/plc4x/pull/545#discussion_r1064040681
##########
plc4go/examples/read/hello_world_plc4go_read.go:
##########
@@ -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")
Review Comment:
We should probably externalize this because every one is probably going to have the device at a different IP
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a diff in pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on code in PR #545:
URL: https://github.com/apache/plc4x/pull/545#discussion_r1064040882
##########
plc4go/protocols/modbus/readwrite/model/DataItem.go:
##########
@@ -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.
-func DataItemParse(readBuffer utils.ReadBuffer, dataType ModbusDataType, numberOfValues uint16) (api.PlcValue, error) {
+func DataItemParse(readBuffer utils.ReadBuffer, dataType ModbusDataType, numberOfValues uint16, offset uint16) (api.PlcValue, error) {
readBuffer.PullContext("DataItem")
switch {
case dataType == ModbusDataType_BOOL && numberOfValues == uint16(1): // BOOL
- // Reserved Field (Just skip the bytes)
- if _, _err := readBuffer.ReadUint16("reserved", 15); _err != nil {
- return nil, errors.Wrap(_err, "Error parsing reserved field")
- }
-
- // Simple Field (value)
- 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)))
Review Comment:
In this branch "numberOfValues" is always 1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4go): Implementing the correct reading of BOOL types
Posted by "sruehl (via GitHub)" <gi...@apache.org>.
sruehl commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1733799059
@chrisdutz @hongjinlin as far as I can see this PR was ready to be merged? Seems then only a merge/rebase might be necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1367196534
Hi,
sorry for coming late to the party ... was insanely busy on my side and I just came back from a holiday break, going through the open Pull Requests.
Could you please update your PR? I'd love to finialize it and merge it as soon as possible.
Chris
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [plc4x] hongjinlin commented on pull request #545: feat(plc4go): Implementing the correct reading of BOOL types
Posted by GitBox <gi...@apache.org>.
hongjinlin commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1374359457
@chrisdutz
Hi Chris,
I have updated it, and have a commit([17d7f7](https://github.com/apache/plc4x/commit/17d7f765c670f86c3fd110f010a3faafe8ee1c5a)) about it, please review it when you have time.
Jinlin
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(plc4go): Implementing the correct reading of BOOL types
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #545:
URL: https://github.com/apache/plc4x/pull/545#issuecomment-1733804228
No objections to merging
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org