-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix write multiple coils byte count validation #2974
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
Changes from 3 commits
4841635
d6e28f6
cb5511d
f1cc9af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,6 +136,9 @@ class WriteMultipleCoilsRequest(ModbusPDU): | |
|
|
||
| function_code = 15 | ||
| rtu_byte_count_pos = 6 | ||
| count: int | ||
| byte_count: int | None = None | ||
| data_byte_count = 0 | ||
|
|
||
| def encode(self) -> bytes: | ||
| """Encode write coils request.""" | ||
|
|
@@ -149,14 +152,23 @@ def encode(self) -> bytes: | |
|
|
||
| def decode(self, data: bytes) -> None: | ||
| """Decode a write coils request.""" | ||
| self.address, count, _byte_count = struct.unpack(">HHB", data[0:5]) | ||
| self.bits = unpack_bitstring(data[5:])[:count] | ||
| self.address, self.count, self.byte_count = struct.unpack(">HHB", data[0:5]) | ||
| self.data_byte_count = len(data) - 5 | ||
| self.bits = unpack_bitstring(data[5 : 5 + self.byte_count])[: self.count] | ||
|
|
||
| async def datastore_update( | ||
| self, context: ModbusServerContext, device_id: int | ||
| ) -> ModbusPDU: | ||
| """Run a request against a datastore.""" | ||
| count = len(self.bits) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is very confusing to have a class variable self.count and a local variable count.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. The local count variable has been removed, and self.count is now used consistently. |
||
| if self.byte_count is not None: | ||
| expected_byte_count = (self.count + 7) // 8 | ||
| if ( | ||
| self.byte_count != expected_byte_count | ||
| or self.data_byte_count != self.byte_count | ||
| ): | ||
| return ExceptionResponse(self.function_code, ExcCodes.ILLEGAL_VALUE) | ||
| count = self.count | ||
| rc = await context.async_setValues( | ||
| device_id, self.function_code, self.address, self.bits | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All 3 have values that depends on the actual request.
Having those as class variables creates a potential problem, when used in parallel. This is one of the rare cases where init is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The three request-dependent values are now initialized per instance in init.