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

Waveform records ignore NELM keyword if initial_value is set #37

Closed
AlexanderWells-diamond opened this issue Sep 14, 2021 · 2 comments · Fixed by #79
Closed

Waveform records ignore NELM keyword if initial_value is set #37

AlexanderWells-diamond opened this issue Sep 14, 2021 · 2 comments · Fixed by #79
Assignees
Labels

Comments

@AlexanderWells-diamond
Copy link
Collaborator

AlexanderWells-diamond commented Sep 14, 2021

If you specify both an initial_value=... and NELM=... keywords when creating a WaveformOut record, the NELM keyword is ignored and the waveform is created with a maximum length equal to the length of the data.

I think the offending line is L169 of builder.py, which seems to override the passed in NELM without checking if it is set:
fields['NELM'] = length

There is a workaround: instead of initial_value and NELM, use something like this (assuming data is a numpy array)

length=required_length,
datatype=data.dtype,

And later do myrecord.set(data)

@Araneidae
Copy link
Member

There are I think two or maybe three issues here:

  1. The waveform initialisation API is a little bit too tricksy, and the documentation
    If ``value`` is specified or if an `initial_value` is specified (only one of
    these can be used) the value is used to initialise the waveform and to
    determine its field type and length. If no initial value is specified then
    the keyword argument ``length`` must be used to specify the length of the
    waveform.
    The field type can be explicitly specified either by setting the ``datatype``
    keyword to a Python type name, or by setting ``FTVL`` to the appropriate EPICS
    field type name. Otherwise the field type is taken from the initial value
    if given, or defaults to ``'FLOAT'``.
    feels a little thin on this point.
  2. The NELM field is being silently overwritten. This is a clear bug.
  3. Perhaps it should be possible to override the length specified in the initial_value with a new length parameter (subject, I would think to the requirement that the new length be large enough for the value).

I guess the next step is to work out what the initialisation API should be, preferably without breaking reasonable existing usage.

@AlexanderWells-diamond
Copy link
Collaborator Author

AlexanderWells-diamond commented Sep 14, 2021

Regarding (1), is there a reason that value, length and datatype keyword arguments exist, while the EPICS keywords initial_value, NELM and FTVL also exist and could be specified on record creation? I can sort of see the reason for datatype, to allow specifying numpy dtypes and have pythonSoftIOC convert it for you, but having two different ways of giving the value and the maximum length seems to just invite issues. It would be nice to know why these exist.

Although at this stage I suppose we have to keep value and length, as they're in the documentation and so we won't want to break compatibility.

Regarding (2), I think the fix is simply:

# If NELM has been explicitly set use that, otherwise use computed length
if 'NELM' not in fields:
    fields['NELM'] = length
elif fields['NELM'] < length:
    raise ValueError("NELM value shorter than value length")

Regarding (3), that's my exact use-case right now. I need to create a very long but mostly empty Waveform, using a short initial_value array and a much larger NELM value. I can accomplish this with the above code snippet and providing initial_value and NELM when I create the record. I cannot currently do it using the value and length keywords. Should I implement a fix that allows both pairs? Or what about mixing the pairs i.e. initial_value and length, or value and NELM?

Araneidae added a commit that referenced this issue Feb 3, 2022
AlexanderWells-diamond added a commit that referenced this issue Feb 3, 2022
AlexanderWells-diamond added a commit that referenced this issue Feb 3, 2022
Other fails appear to be the spawned Validate Process not correctly
updating, or possibly not being removed from previous test run, on each
iteration. Workaround is to give each one a unique device prefix and
add logging to work out what it might be for future issues.
AlexanderWells-diamond added a commit that referenced this issue Feb 3, 2022
Other fails appear to be the spawned Validate Process not correctly
updating, or possibly not being removed from previous test run, on each
iteration. Workaround is to give each one a unique device prefix and
add logging to work out what it might be for future issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment