plc4x icon indicating copy to clipboard operation
plc4x copied to clipboard

feat(plc4go): Implementing the correct reading of BOOL types

Open hongjinlin opened this issue 3 years ago • 10 comments

Implemented according to Cleanup of how we handle all the bit-related fields

Thanks for reviewing the code.

hongjinlin avatar Oct 13 '22 06:10 hongjinlin

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 ...

chrisdutz avatar Oct 13 '22 13:10 chrisdutz

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

chrisdutz avatar Dec 29 '22 09:12 chrisdutz

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.

hongjinlin avatar Dec 29 '22 10:12 hongjinlin

@chrisdutz Hi Chris,

I have updated it, and have a commit(https://github.com/apache/plc4x/commit/17d7f765c670f86c3fd110f010a3faafe8ee1c5a) about it, please review it when you have time.

Jinlin

hongjinlin avatar Jan 07 '23 02:01 hongjinlin

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.

chrisdutz avatar Jan 07 '23 19:01 chrisdutz

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

hongjinlin avatar Jan 09 '23 03:01 hongjinlin

Always happy to help educate :-)

chrisdutz avatar Jan 09 '23 09:01 chrisdutz

@chrisdutz @hongjinlin as far as I can see this PR was ready to be merged? Seems then only a merge/rebase might be necessary.

sruehl avatar Sep 25 '23 14:09 sruehl

No objections to merging

chrisdutz avatar Sep 25 '23 14:09 chrisdutz

@hongjinlin can you merge main into this branch or rebase?

sruehl avatar Sep 25 '23 14:09 sruehl

@sruehl what do you think? You want to migrate this manually or should we close and drop it?

chrisdutz avatar Jun 28 '24 11:06 chrisdutz