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

Draft: Add PythonSoftIOC record creation directly from PandA #28

Closed
wants to merge 210 commits into from

Conversation

AlexanderWells-diamond
Copy link
Contributor

Note this is very much a work in progress, and it currently isn't functional.

…inside this module.

The _ioc.py file will not live here forever, it's just easier to run and debug it from outside of the module structure, as it avoids some name clashing errors.
…s, and values.

Also contains the start of an attempted mapping from (type, subtype) -> function to create record.
…e mechanism.

Note this doesn't work at the moment - the second GetChanges call (in update()) hangs indefinitely. I don't know why...
@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2021

Codecov Report

Base: 96.24% // Head: 96.43% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (a869aa0) compared to base (4e7a1ef).
Patch coverage: 96.29% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
+ Coverage   96.24%   96.43%   +0.18%     
==========================================
  Files          11       15       +4     
  Lines         932     2019    +1087     
==========================================
+ Hits          897     1947    +1050     
- Misses         35       72      +37     
Impacted Files Coverage Δ
pandablocks/_control.py 85.39% <ø> (ø)
pandablocks/cli.py 91.95% <82.14%> (-3.77%) ⬇️
pandablocks/hdf.py 97.27% <83.33%> (-2.73%) ⬇️
pandablocks/ioc/ioc.py 94.20% <94.20%> (ø)
pandablocks/ioc/_hdf_ioc.py 98.40% <98.40%> (ø)
pandablocks/ioc/_tables.py 99.03% <99.03%> (ø)
pandablocks/asyncio.py 97.89% <100.00%> (+0.04%) ⬆️
pandablocks/blocking.py 97.43% <100.00%> (+0.03%) ⬆️
pandablocks/commands.py 97.77% <100.00%> (+3.35%) ⬆️
pandablocks/ioc/__init__.py 100.00% <100.00%> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Note that the update() method will basically always throw exceptions as there are no records created for the fields that most commonly change.
…implementation still doesn't let me call Get() successfully.
…fy a command saving data into FieldInfo

It revolves around the _FieldCommandMapping class, which links together a command and the attribute in a FieldInfo class. It also handes converting the strings from PandA into more useful Python types.

The FieldInfo class and GetFieldInfo command are being slowly expanded with new attributes for various field types. At some point in the future they'll need to be broken apart.
This results in a very bloated FieldInfo structure - I'll probably sub-class it for each Type to trim it down.

Also add some notes to _ioc.py.
… already captured in the `labels` field (which is the logic pymalcolm uses)
…butes.

Clear out commented out code and attributes.
Also a lot of other miscellaneous refactoring and comments
NOTE: Something is causing pytest to hang indefinitely after finishing tests, which is causing all kinds of problems. Investigating now...
…es a new event loop that never gets terminated
Fix associated tests.
Start work on test_get_fields_parameterized_type to test every type permutation.
Doing this after discussion with Tom, who said that there was a historical reason that these two CAPTURE attributes were treated differently in pymalcolm (which this code is based on), but that difference will be disappearing soon and so they should be treated the same.
Add missing cases to GetFieldInfo's dictionary lookup.

Note there's a bug somewhere in _make_scalar - PythonSoftIOC complains that the initial_value's type is wrong when it creates the main record, hence it's commented out at the moment.
Also identified problems with the `update` method - we can't use a string type to set most of these records. We have to know what type to set, and in the case of mbbi/mbbo records we have to set the index...
This should have been done as part of commit
9609161
When a record is updated, it Puts the value to PandA, which then returns
the value back to us on our next *CHANGES? call. We must stop the value
from being re-set on the record if that value was just Put.

This required a lot of rearchitecture. The RecordInfo structure is the
only structure that is available in the *CHANGES handler, and so I
refactored so that it is available in the RecordUpdater.
This presented some circular dependency issues - the RecordUpdater must
be created before the record, so the RecordInfo must be created before
that, but then the record itself needs to live in the RecordInfo so
must be put in at a later time.
This reduces the number of warnings due to overlong enum labels during
startup.

We cannot so easily do this for mbbOut records as they can be set
through EPICS, and so need to conform to the expected enums.
@AlexanderWells-diamond
Copy link
Contributor Author

AlexanderWells-diamond commented Sep 5, 2022

* Again on the `FieldInfo` class: The fields of the child classes are all optional. This is due to the `dataclass` restriction that once there is an optional field (the description in the base `FieldInfo` may not exist) all subsequently defined fields must also be optional. However we know that these fields must exist, and this leads to a large amount of `assert` statements throughout the code to make `mypy` happy. The solution I'm aware of is to split the classes into smaller dataclasses, grouping together the required and the optional parameters, and then stitch them together on a class-by-class basis to create the right order of parameters. This would mean the right fields would be required, but it would also make it a lot harder to inspect a class and know what fields were in it as they'd be inherited from two or three different classes.

Can this be solved by making all the base class fields required, then passing them a junk value? Otherwise we end up with something like https://stackoverflow.com/questions/51575931/class-inheritance-in-python-3-7-dataclasses/73095470#73095470 which doesn't actually help with the mypy issue

I must have misunderstood what was causing me the problem originally: It wasn't the Optional, it was actually the default value. Removing them from the base FieldInfo allowed me to remove all the incorrect Optional markers in all the subclasses. Done in 9609161 and df7bdd8.

* Due to PandA reporting any changes we do through `Put` commands back to us through `GetChanges`, we end up setting record values twice. I'm unsure of the full implications of this - I disable processing for Out records, and In records don't have `validate` or `on_update` methods anyway, but tools like `camonitor` may report changes, especially if any records have `always_update` set. This may also pose issues for fast-changing records, as the polling in `GetChanges` will only happen periodically and so could easily miss updates.

I do this at the moment by setting the a flag that ignores the next update if the new value is equal to the value we set: https://github.com/dls-controls/pymalcolm/blob/a9caeccf5b75c444f653f6785edef27599e4b60b/malcolm/modules/pandablocks/parts/pandafieldpart.py#L60-L75

Implemented in adf2713. This required a fair chunk of re-architecting, to allow access to the RecordInfo structure in both the EPICS on-update callback and the processing of *CHANGES? requests. It's pretty ugly, but is the simplest way to achieve the required result.

* Lastly on logging, on my test PandA we see a lot of warnings due to label lengths exceeding what can be stored in an `mbbi/mbbo` record. These are all `warning` level log messages. We did discuss a similar thing for the `description` truncation messages, which have been bumped down to the `info` log level. A possible thing to do here is to use `string` records, rather than `mbbi/mbbo` records, and apply the [validator class](https://github.com/PandABlocks/PandABlocks-client/blob/pythonsoftioc/pandablocks/ioc/ioc.py#L359) I wrote for them.

I think we should change all the read-only enums to string records regardless off their length, which we can wrap in as a PVA enum later. That will catch most of them.

I changed the read-only enum (for param-read ) with a string record. All other record types are mbbOut, and so cannot so easily be changed. See 193dbd8.

Also add test for setting errors on records.

At this point there's only one or two lines of functional code that
isn't tested - the rest is exception logging
When the PandA updates the size of its tables, the DRVH value of the
INDEX record will be set to the maximum allowed value

Note that this does not happen immediately when data is added from the
EPICS side - we rely on the data being pushed to the PandA, and then it
being reported back from a *CHANGES request?
When a channel connection is broken, aioca will issue an exception.
By cleaning the cache at all opportunities we suppress the errors.
This is a more sensible place for the data, which still has to be
instantiated at runtime, due to needing a reference to the class
instance, but makes more sense than creating it inline
@AlexanderWells-diamond
Copy link
Contributor Author

@coretl I think I've now addressed all the points in your review. There's still a couple of conversations I've left unresolved where I'm waiting for additional feedback/discussion.

I decided not to refactor the _make commands in the end - I could have saved a few lines in most functions by moving the "expected values" check and possibly some other parts too, but it became more complicated due to several special cases.

I also implemented several old TODOs, which added minor functions like the DRVH of a table's INDEX record is now updated dynamically.

pandablocks/ioc/ioc.py Show resolved Hide resolved
pandablocks/ioc/ioc.py Outdated Show resolved Hide resolved
pandablocks/ioc/ioc.py Show resolved Hide resolved
This is up from 1Hz. Fixed tests to accomodate this.
The value of the Action field is immaterial, all that matters is that it
was set
The PandA may update bit_out values much faster than our polling period.
To represent this, we will toggle the record's value, and reset it on
the next loop of update()
This removes the need to clamp the values to the max of a 32-bit integer
Instead we have the full value of a float64 to represent the value,
which allows us to represent the full range of PandA values
@AlexanderWells-diamond
Copy link
Contributor Author

@coretl Think I'm done with review comments again.

This means the relevant command line options will only appear if the
required extra modules have been installed.
Without this, it was possible to run the hdf5 command line but it would
just produce an ImportError
By providing the functions as hidden, they won't appear in any help
output but will run if directly executed. This'll print a more helpful
error for users who may be following a guide and not realizing they need
to specifically install the extras.

Alternative mechanisms like using `logging` don't work well as it means
it'll either always or never print the message.
@AlexanderWells-diamond
Copy link
Contributor Author

This has migrated to PandABlocks-ioc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants