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

S7: changed byteLength and blockNumber from short to int #44

Merged
merged 1 commit into from
Jan 16, 2019

Conversation

timbo2k
Copy link
Contributor

@timbo2k timbo2k commented Jan 16, 2019

byteLength and blockNumbers had been to small using short for possible values
added some tests for out of range values
did successful manual test with scraper and updated commited yml-config

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.

Hi Tim,
it seems that the block number still is a short. It's just the byteOffset that is 21 bits big.
Could you please adjust your PR to this?

Chris

@timbo2k
Copy link
Contributor Author

timbo2k commented Jan 16, 2019

Hi Chris

I think there are also block number > 32000 possible (see attached screenshot from a recently added block with the number 50000), so Short will not be enough in my opinion, this maybe depends on the part of used S7 device --> I wrote a ToDo for that in the code
bildschirmfoto 2019-01-16 um 21 24 02

@chrisdutz
Copy link
Contributor

Argh ... dammit ... you are right. The protocol only transfers 2 bytes (1 short). However you are correct that Java shorts are signed and the protocol uses unsigned, so you are correct that we should change this to int ... however we now have to check if the protocol code is correct everywhere.

@chrisdutz
Copy link
Contributor

Ok .. so I double-checked the Protocol implementaton ... on the writing side, we're safe as "writeShort(int)" works like a charm and we currently don't parse "AnyItems", so no way we could parse it wrong. Guess we can merge this.

Thanks for contributing :-)

👍

@chrisdutz chrisdutz merged commit 7031908 into apache:develop Jan 16, 2019
@timbo2k
Copy link
Contributor Author

timbo2k commented Jan 16, 2019

I just ran a test vs our physical device with had been successful - so everything seems to be Ok

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.

2 participants