Skip to content

Fix #628: hex values for repl arguments #1043

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.

Sign up for GitHub

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

Closed
pkubanek wants to merge 1 commit into from
Closed

Fix #628: hex values for repl arguments #1043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions pymodbus/bit_write_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
# pylint: disable=missing-type-doc
import struct

from pymodbus.constants import ModbusStatus
from pymodbus.constants import Defaults, ModbusStatus
from pymodbus.pdu import ModbusExceptions as merror, ModbusRequest, ModbusResponse
from pymodbus.utilities import pack_bitstring, unpack_bitstring

Expand Down Expand Up @@ -37,13 +37,14 @@ class WriteSingleCoilRequest(ModbusRequest):
function_code = 5
_rtu_frame_size = 8

def __init__(self, address=None, value=None, **kwargs):
def __init__(self, address=None, value=None, unit=Defaults.UnitId, **kwargs):
"""Initialize a new instance.

:param address: The variable address to write
:param value: The value to write at address
:param unit: Modbus slave unit ID
"""
ModbusRequest.__init__(self, **kwargs)
super().__init__(unit, **kwargs)
self.address = address
self.value = bool(value)

Expand Down Expand Up @@ -107,13 +108,14 @@ class WriteSingleCoilResponse(ModbusResponse):
function_code = 5
_rtu_frame_size = 8

def __init__(self, address=None, value=None, **kwargs):
def __init__(self, address=None, value=None, unit=Defaults.UnitId, **kwargs):
"""Initialize a new instance.

:param address: The variable address written to
:param value: The value written at address
:param unit: Modbus slave unit ID
"""
ModbusResponse.__init__(self, **kwargs)
super().__init__(unit, **kwargs)
self.address = address
self.value = value

Expand Down Expand Up @@ -160,13 +162,14 @@ class WriteMultipleCoilsRequest(ModbusRequest):
function_code = 15
_rtu_byte_count_pos = 6

def __init__(self, address=None, values=None, **kwargs):
def __init__(self, address=None, values=None, unit=Defaults.UnitId, **kwargs):
"""Initialize a new instance.

:param address: The starting request address
:param values: The values to write
:param unit: Modbus slave unit ID
"""
ModbusRequest.__init__(self, **kwargs)
super().__init__(unit, **kwargs)
self.address = address
if not values:
values = []
Expand Down Expand Up @@ -241,13 +244,14 @@ class WriteMultipleCoilsResponse(ModbusResponse):
function_code = 15
_rtu_frame_size = 8

def __init__(self, address=None, count=None, **kwargs):
def __init__(self, address=None, count=None, unit=Defaults.UnitId, **kwargs):
"""Initialize a new instance.

:param address: The starting variable address written to
:param count: The number of values written
:param unit: Modbus slave unit ID
"""
ModbusResponse.__init__(self, **kwargs)
super().__init__(unit, **kwargs)
self.address = address
self.count = count

Expand Down
25 changes: 15 additions & 10 deletions pymodbus/client/mixin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,59 +104,64 @@ def read_coils(self, address, count=1, unit=Defaults.UnitId, **kwargs):
request = ReadCoilsRequest(address, count, unit, **kwargs)
return self.execute(request) # pylint: disable=no-member

def read_discrete_inputs(self, address, count=1, **kwargs):
def read_discrete_inputs(self, address, count=1, unit=Defaults.UnitId, **kwargs):
"""Read discrete inputs.

:param address: The starting address to read from
:param count: The number of discretes to read
:param unit: Modbus slave unit ID
:param kwargs:
:returns: A deferred response handle
"""
request = ReadDiscreteInputsRequest(address, count, **kwargs)
request = ReadDiscreteInputsRequest(address, count, unit, **kwargs)
return self.execute(request) # pylint: disable=no-member

def write_coil(self, address, value, **kwargs):
def write_coil(self, address, value, unit=Defaults.UnitId, **kwargs):
"""Write_coil.

:param address: The starting address to write to
:param value: The value to write to the specified address
:param unit: Modbus slave unit ID
:param kwargs:
:returns: A deferred response handle
"""
request = WriteSingleCoilRequest(address, value, **kwargs)
request = WriteSingleCoilRequest(address, value, unit, **kwargs)
return self.execute(request) # pylint: disable=no-member

def write_coils(self, address, values, **kwargs):
def write_coils(self, address, values, unit=Defaults.UnitId, **kwargs):
"""Write coils.

:param address: The starting address to write to
:param values: The values to write to the specified address
:param unit: Modbus slave unit ID
:param kwargs:
:returns: A deferred response handle
"""
request = WriteMultipleCoilsRequest(address, values, **kwargs)
request = WriteMultipleCoilsRequest(address, values, unit, **kwargs)
return self.execute(request) # pylint: disable=no-member

def write_register(self, address, value, **kwargs):
def write_register(self, address, value, unit=Defaults.UnitId, **kwargs):
"""Write register.

:param address: The starting address to write to
:param value: The value to write to the specified address
:param unit: Modbus slave unit ID
:param kwargs:
:returns: A deferred response handle
"""
request = WriteSingleRegisterRequest(address, value, **kwargs)
request = WriteSingleRegisterRequest(address, value, unit, **kwargs)
return self.execute(request) # pylint: disable=no-member

def write_registers(self, address, values, **kwargs):
def write_registers(self, address, values, unit=Defaults.UnitId, **kwargs):
"""Write registers.

:param address: The starting address to write to
:param values: The values to write to the specified address
:param unit: Modbus slave unit ID
:param kwargs:
:returns: A deferred response handle
"""
request = WriteMultipleRegistersRequest(address, values, **kwargs)
request = WriteMultipleRegistersRequest(address, values, unit, **kwargs)
return self.execute(request) # pylint: disable=no-member

def read_holding_registers(self, address, count=1, unit=Defaults.UnitId, **kwargs):
Expand Down
21 changes: 13 additions & 8 deletions pymodbus/register_write_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# pylint: disable=missing-type-doc
import struct

from pymodbus.constants import Defaults
from pymodbus.pdu import ModbusExceptions as merror, ModbusRequest, ModbusResponse


Expand All @@ -16,13 +17,14 @@ class WriteSingleRegisterRequest(ModbusRequest):
function_code = 6
_rtu_frame_size = 8

def __init__(self, address=None, value=None, **kwargs):
def __init__(self, address=None, value=None, unit=Defaults.UnitId, **kwargs):
"""Initialize a new instance.

:param address: The address to start writing add
:param value: The values to write
:param unit: Modbus slave unit ID
"""
super().__init__(**kwargs)
super().__init__(unit, **kwargs)
self.address = address
self.value = value

Expand Down Expand Up @@ -85,13 +87,14 @@ class WriteSingleRegisterResponse(ModbusResponse):
function_code = 6
_rtu_frame_size = 8

def __init__(self, address=None, value=None, **kwargs):
def __init__(self, address=None, value=None, unit=Defaults.UnitId, **kwargs):
"""Initialize a new instance.

:param address: The address to start writing add
:param value: The values to write
:param unit: Modbus slave unit ID
"""
super().__init__(**kwargs)
super().__init__(unit, **kwargs)
self.address = address
self.value = value

Expand Down Expand Up @@ -145,13 +148,14 @@ class WriteMultipleRegistersRequest(ModbusRequest):
_rtu_byte_count_pos = 6
_pdu_length = 5 # func + adress1 + adress2 + outputQuant1 + outputQuant2

def __init__(self, address=None, values=None, **kwargs):
def __init__(self, address=None, values=None, unit=Defaults.UnitId, **kwargs):
"""Initialize a new instance.

:param address: The address to start writing to
:param values: The values to write
:param unit: Modbus slave unit ID
"""
super().__init__(**kwargs)
super().__init__(unit, **kwargs)
self.address = address
if values is None:
values = []
Expand Down Expand Up @@ -230,13 +234,14 @@ class WriteMultipleRegistersResponse(ModbusResponse):
function_code = 16
_rtu_frame_size = 8

def __init__(self, address=None, count=None, **kwargs):
def __init__(self, address=None, count=None, unit=Defaults.UnitId, **kwargs):
"""Initialize a new instance.

:param address: The address to start writing to
:param count: The number of registers to write to
:param unit: Modbus slave unit ID
"""
super().__init__(**kwargs)
super().__init__(unit, **kwargs)
self.address = address
self.count = count

Expand Down
8 changes: 4 additions & 4 deletions pymodbus/repl/client/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,19 +146,19 @@ def _process_args(args, string=True):
if not string:
if "," in val:
val = val.split(",")
val = [int(v) for v in val]
val = [int(v, 0) for v in val]
Copy link
Contributor

@dhoomakethu dhoomakethu Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this helping?

From the docs Base 0 means to interpret exactly as a code literal, so that the actual base is 2, 8, 10, or 16, and so that int('010', 0) is not legal, while int('010') is, as well as int('010', 8).

Copy link
Contributor

@dhoomakethu dhoomakethu Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share some outputs with out this change and with this change please?
pymodbus.console >> client.read_holding_registers unit=0x1 count=0x20 address=0x30 is this something you are trying to achieve here ?

Copy link
Collaborator

@janiversen janiversen Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a short review of your code, and I agree with @dhoomakethu it do look strange. Your idea is OK, but you need to work on the implementation (and solve your rebase problem).

Copy link
Contributor Author

@pkubanek pkubanek Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int([str], 0) takes any sensible value as numerical. The original request was for accepting hex values. int([str], 0) accepts any value, with the cost that int('010', 0) throws ValueException - that can be handled by catching it and trying int([str]), perhaps in a function, as this is on multiple places in the code.

I am not sure about rebasing issues. I discovered problems in my previous PR (#1041), which I fixed with this. Do you want to make an extra PR for improving those?

Copy link
Collaborator

@janiversen janiversen Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I already told you, your problems come from the fact that you use dev to submit PRs. You need to submit a PR based on the latest dev, whether you do it in this PR or open a new PR is up to you. I gave you a recipe how to work with dev and branches for each PR.

You need to prove that your PR accept hex values without causing problems. I believe that '010' is accepted today (without having tested it), so it needs to be accepted in this PR as well.

The best way to show your changes works as expected, is to write some test cases in test/test_.py

Copy link
Contributor Author

@pkubanek pkubanek Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Let's close this one, I will make two new PRs, one for fixing problems created by #1041, second to address #628, with still accepting 010 and providing tests to show that.

Copy link
Collaborator

@janiversen janiversen Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not aware that there are any problems due to #1041, I suggest you solve your dev problem first. If there are problems I look forward to see a PR.

#628 is a long standing issue, so it will be nice to have that fixed.

else:
val = int(val)
val = int(val, 0)
kwargs[arg_name] = val
else:
arg_name, val = arg, args[i + 1]
try:
if not string:
if "," in val:
val = val.split(",")
val = [int(v) for v in val]
val = [int(v, 0) for v in val]
else:
val = int(val)
val = int(val, 0)
kwargs[arg_name] = val
skip_index = i + 1
except TypeError:
Expand Down