min_value option for time fields is being read incorrectly

[tomtrafford]

The server should try to parse a min_value (preceded by a '>' character) of Time fields in the config file. Currently this leads to an unexpected character error. After discussing this with @Araneidae, there is a bug in the server as it is trying to parse the min_value from the registers file instead.

This functionality is not currently used, is it worth implementing correctly or should this be removed. @coretl Are there use cases for this to be implemented correctly?

[Araneidae]

The documentation clearly describes this as part of the field type (rather than subtype) here:

PandABlocks-server/docs/config.rst

Lines 99 to 105 in e5a5dab

	``time`` [ ``>`` min_value ]
	Time fields behave like ``param`` fields, but need special treatment because
	the underlying value is 64-bits and so two registers need to be written.
	If desired a minimum valid value can be specified as `min_value`. This will
	prevent the writing of values less than this value and can be read as the
	``.MIN`` attribute.

but alas the implementation, as @tomtrafford says, parses this as part of the register file parsing here:

PandABlocks-server/server/time.c

Lines 108 to 111 in e5a5dab

	IF(**line != '\0',
	parse_whitespace(line) ?:
	parse_char(line, '>') ?:
	parse_uint64(line, &state->min_value));

We can fix the documentation, fix the implementation, or drop the functionality altogether. In order, I'd prefer to drop the functionality or fix the implementation if necessary; this behaviour is part of the user interface (and it can be interrogated via the .MIN attribute), so definitely belongs in config, not register. However, if this is obsolete and unused it ought to go! We'd have to remove .MIN at the same time, of course.

[Araneidae]

@coretl, do you have a preference here? I'd prefer to drop support for this feature altogether as it doesn't appear to be used.

[coretl]

There are a couple of cases where the field must be non-zero, but generally we just treat 0 as "the shortest possible time period", so I'm happy to stick with that behaviour and remove this feature

[Araneidae]

The code always guards against writing 0 to time fields, and I haven't changed that, here's the relevant part of the diff:

diff --git a/server/time.c b/server/time.c
@@ -143,7 +135,7 @@ static error__t write_time_value(
 {
     struct time_class_state *state = class_data;
     error__t error =
-        TEST_OK_(value == 0  ||  value > state->min_value, "Value too small");
+        TEST_OK_(value == 0, "Value too small");
     if (!error)
         WITH_MUTEX(state->mutex)
         {

I would push my merge, but PR #55 needs to be approved first.

[coretl]

Well I'm very confused then, because we explicitly use PULSE.WIDTH = 0 as a flag:
https://github.com/PandABlocks/PandABlocks-FPGA/blob/e967856854a5d4face263292523fb4ec8e70dd26/modules/pulse/pulse.block.ini#L14-L17

And I've just testing writing 0 to PULSE.WIDTH on a live PandA and it works...

[coretl]

Wait, doesn't this:

TEST_OK_(value == 0  ||  value > state->min_value, "Value too small");

mean that a value of 0, or a value greater than min_value is fine? In which case deleting the TEST_OK_ line completely seems better?

[Araneidae]

Oops. It's worse than that: my patch now only accepts 0 as an input. I'd better fix that...

[Araneidae]

I do realise now that simply fixing the documentation would have been easier. Oh well, it's not being used, so let's get rid of the feature.