katcp.Sensor, params not formatted properly

[carterb23]

Python 3.7:
Following error on v0.7.0 when setting up a sensor:
sens_type = Sensor.parse_type('float')
test_sens = Sensor( sensor_type=sens_type, name='test', params=['0', '1', '2', '3'])
whereas
test_sens = Sensor( sensor_type=sens_type, name='test', params=[0, 1, 2, 3])
works. Sensor class description says:

@var params
@brief List of strings containing the additional parameters (length and
interpretation are specific to the sensor type).

ERROR:

Traceback (most recent call last):
File "E:/Workspace/Projects/MPIfR/GIM/software/python/katcp_client/client_callback.py", line 29, in
main()
File "E:/Workspace/Projects/MPIfR/GIM/software/python/katcp_client/client_callback.py", line 22, in main
client = KatcpClient(opts.host, opts.port)
File "E:\Workspace\Projects\MPIfR\GIM\software\python\katcp_client\esg3_katcp\katcp_client.py", line 87, in init
params=['0', '1', '2', '3']
File "C:\Program Files (x86)\Python3\lib\site-packages\katcp\core.py", line 1098, in init
self.formatted_params = [self._formatter(p, True) for p in params]
File "C:\Program Files (x86)\Python3\lib\site-packages\katcp\core.py", line 1098, in
self.formatted_params = [self._formatter(p, True) for p in params]
File "C:\Program Files (x86)\Python3\lib\site-packages\katcp\kattypes.py", line 115, in pack
return self.encode(value, major)
File "C:\Program Files (x86)\Python3\lib\site-packages\katcp\kattypes.py", line 205, in encode
raise ValueError("Could not encode value '%r' as float." % value)
ValueError: Could not encode value ''0'' as float.

[ajoubertza]

Hi @carterb23

Thanks for the report. I think the behaviour is correct, but the docstring is confusing. For a float sensor, the parameters (which are optional) are the minimum and maximum values the float sensor is expected to have. They need to be numeric (so int or float).

The docstring for the Sensor class mentions params twice. The first one is correct (although it doesn't mention the type of the items in the list explicitly).

First mention:

katcp-python/katcp/core.py

Lines 917 to 925 in b3468dc

	params : list
	Additional parameters, dependent on the type of sensor:
	* For :const:`INTEGER` and :const:`FLOAT` the list should
	give the minimum and maximum that define the range
	of the sensor value.
	* For :const:`DISCRETE` the list should contain all
	possible values the sensor may take.
	* For all other types, params should be omitted.

Second mention:

katcp-python/katcp/core.py

Lines 1029 to 1031 in b3468dc

	## @var params
	# @brief List of strings containing the additional parameters (length and
	# interpretation are specific to the sensor type).

The first mention is also the one that ends up on readthedocs:
https://katcp-python.readthedocs.io/en/v0.7.0/api.html#sensor

I will try to make the documentation a little clearer.

Other notes:

I'm not sure if you specifically want to create the sensors with the type dynamically, thus the use of Sensor.parse_type('float'). If not, there is a shorter way of instantiating the float sensors:

test_sens = Sensor.float(name='test', params=[0, 1])

Also note that the min and maximum parameters for float and int sensors don't do much. They are only for information, and don't limit the values assigned to the sensor. Supplying more than 2 parameters for a float sensor doesn't make sense either. Either provide 2 - the min and max, or provide none.

[ajoubertza]

@carterb23 I'm going to close this issue tomorrow, unless you still have questions or comments.

[carterb23]

@ajoubertza: Thanks for the info! Wouldn't it be more consistent to implement the convert/cast functionality in the Sensor class. There you have all the information needed and it is done on many other places in the module the same way (passing string value and conversion/cast is done in the class). From my understanding, dealing with sensor types fits best in the Sensor class (but that's my opinion).

[ajoubertza]

Yes, the params could be more forgiving and do a some validation on the type. As for consistency, many of the methods in this core module are strict about the types, so I'm not convinced that the Sensor constructor is going against the established pattern.

You're welcome to submit a pull request if you feel strongly that the behaviour of params initialisation should be changed.

[carterb23]

I thought the same. I need this functionality for my app either way. Let's see how it goes.