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

Panda conversion improvements #72

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

evalott100
Copy link
Contributor

@evalott100 evalott100 commented Nov 11, 2024

Changes

Attributes

Mapping

Controllers can now define

    @property
    def additional_attributes(self) -> dict[str, Attribute]:
        """FastCS will look for attributes on the controller, but additional attributes
        are provided by this method."""
        return {}

which is another way to provide attributes without setattr on the controller. The keys are checked to be distinct from the attributes found from the attributes of the controller.

The user can also pass in search_device_for_attributes=False on the controller which will prevent the mapping automatically searching for Attributes, it will only use additional_attributes.

Description

Added a field called description to Attribute, which is used as DESC on the EPICS record.

Initial value

Added an option for initial_value on SignalR/SignalRW. If provided this will be used instead of the default of the _datatype.dtype for the initial value of the attribute.

Changing the datatype

We still want DataType to be frozen, but we also might want to change the value of a field in it - e.g units, corresponding to EGU in the epics backend.

To do this I've made a method called update_datatype which will change the _datatype of the attribute. This alone wouldn't update the corresponding record, so the backend runs set_update_databack_callback. update_datatype will then run this so changing the attribute datatype will update the backend.

Scan Tasks

Errors

Currently, errors in a scan future will be swallowed. Now an error will be printed.

Killing Scan Tasks

We were getting problems for scan tasks not being torn down after the end of tests. Now the backend will stop the tasks on __del__, or running the new stop_scan_tasks method.

DataTypes

Added metadata to the datatype

Added a bunch of optional fields to String, Int, Float which are used for relevant fields e.g HOPR in the epics backend. Int and Float Attributes are validated against their max and min on put.

New Datatypes (PENDING)

Waveform

A waveform to correspond to a single pv without any pva structure beyond regular pvi.

Table

A waveform which is indexable using pvi PVs generated from names of the columns of the numpy structured datatype.

Epics Backend

Options

Naming conventions are now passed into the ioc through EpicsIOCOptions, which can be passed into the backend on init.

Handlers

Poll Period

update_period is now optional in fields, designating that the Updater shouldn't be ran periodically. In panda we get all the changes at once in the top level update scan method. We want to then update all the relevant attributes from there.

@evalott100 evalott100 self-assigned this Nov 11, 2024
@evalott100 evalott100 marked this pull request as draft November 11, 2024 14:21
@evalott100
Copy link
Contributor Author

evalott100 commented Nov 12, 2024

Scan tasks are not ending correctly in the tests on main:

========================================================== 30 passed in 2.93s ===========================================================
INFO: PVXS QSRV2 is loaded, permitted, and ENABLED.
Task was destroyed but it is pending!
task: <Task pending name='Task-11' coro=<_create_periodic_scan_task..scan_task() done, defined at /home/eva/Code/FastCS/src/fastcs/backend.py:156> wait_for= cb=[_chain_future.._call_set_state() at /usr/lib64/python3.11/asyncio/futures.py:394]>
Task was destroyed but it is pending!
task: <Task pending name='Task-12' coro=<_create_periodic_scan_task..scan_task() done, defined at /home/eva/Code/FastCS/src/fastcs/backend.py:156> wait_for= cb=[_chain_future.._call_set_state() at /usr/lib64/python3.11/asyncio/futures.py:394]>

Edit

This has been handled in the below commit.

Also made it so that scan tasks are explicitly torn down when the
backend is deleted.
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 77.16263% with 66 lines in your changes missing coverage. Please review.

Project coverage is 79.31%. Comparing base (bdab4fa) to head (3b915ca).

Files with missing lines Patch % Lines
src/fastcs/backends/epics/ioc.py 73.41% 42 Missing ⚠️
src/fastcs/backends/epics/util.py 69.56% 7 Missing ⚠️
src/fastcs/attributes.py 70.58% 5 Missing ⚠️
src/fastcs/backend.py 84.00% 4 Missing ⚠️
src/fastcs/datatypes.py 88.23% 4 Missing ⚠️
src/fastcs/backends/epics/backend.py 70.00% 3 Missing ⚠️
src/fastcs/mapping.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
- Coverage   82.75%   79.31%   -3.45%     
==========================================
  Files          23       23              
  Lines         928     1078     +150     
==========================================
+ Hits          768      855      +87     
- Misses        160      223      +63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

For example, updating an attribute to have an `Int` with different units will make the epics backend update `EGU` of the corresponding record.
@GDYendell
Copy link
Contributor

Looks like this will fix #55

@GDYendell
Copy link
Contributor

This is quite a lot. It would be good to split this into smaller PRs. I think some of it can be merged now and then we can continue developing and discussing the rest. It will also help resolve conflicts with the various PRs @marcelldls is working on in smaller chunks.

@evalott100
Copy link
Contributor Author

This is quite a lot. It would be good to split this into smaller PRs. I think some of it can be merged now and then we can continue developing and discussing the rest.

I agree, I had a philosophy of putting all the changes together since I've amended them so much. Let me know which features you want now and I can cherry pick them into their own issues/PRs

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.

2 participants