Remove tango polling

[marcelldls]

fixes #50

At the time of implementation the attribute polling was deferred to the device server and therefore it had its own event loop

[codecov]

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.56%. Comparing base (bdab4fa) to head (92f5e46).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/fastcs/backends/tango/dsr.py	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
+ Coverage   82.75%   85.56%   +2.80%     
==========================================
  Files          23       23              
  Lines         928      928              
==========================================
+ Hits          768      794      +26     
+ Misses        160      134      -26     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[marcelldls]

Also I have removed "register_dev" from being called on start up. Device registration logic is probably better left to the library consumer

[GDYendell]

This looks good. Just to make sure I understand, is the key thing here removing polling_period= from the tango attribute creation? Does that stop tango from creating poll tasks? What does adding green_mode= do if tango is not doing any polling?

The tests look quite neat, but they are also a bit hard to understand. Could you add some comments / docstrings to the test utilities to explain what they are doing and maybe link to the pytango docs to explain the magic DeviceTestContext is doing?

[marcelldls]

This looks good. Just to make sure I understand, is the key thing here removing polling_period= from the tango attribute creation? Does that stop tango from creating poll tasks? What does adding green_mode= do if tango is not doing any polling?

The tests look quite neat, but they are also a bit hard to understand. Could you add some comments / docstrings to the test utilities to explain what they are doing and maybe link to the pytango docs to explain the magic DeviceTestContext is doing?

Thank you for the feedback Gary. I think I will try get this merged first and then cascade your comments through my other PRs

Id say the core issue is that when I had the task to add a "Tango backend" - I understood that backend implys that Tango needs to do the work. Therefore I had the Tango server:

• Execute the connect method
• Poll the attributes

However, Tango is running its own event loop (I did look at the source and there is no way to pass it an eventloop, maybe theres a way to fool get_event_loop with a context manager of sorts but also afraid of the weird Tango threading) therefore I believe this is why there were issues reported with the locks.

The solution is to not poll the attributes and leave that to FastCS. The tango server is simply given the method to read the attributes - which is what gets triggered by a Tango Client request. So the server is only a thin layer. GreenMode here allows the tango server to execute the awaitable methods like the async Commands

Btw the change from MethodType for commands is because I could not get it testable (doesnt seem to raise call_count?)

Interestingly, the testing is fairly consistent with the testing suggested for FastAPI. I agree it could do with more explanation

[GDYendell]

Ah OK I see, it is the wrapper functions that have changed. So, the wrapped fget now just calls get and not update. Is tango still calling that method periodically, but it is just to sync the value from the fastcs attribute into the tango device?

It might be nicer to use the attribute update_callback to call a tango device function to update the value when it changes, like the EPICS backend does here.

[marcelldls]

Ah OK I see, it is the wrapper functions that have changed. So, the wrapped fget now just calls get and not update. Is tango still calling that method periodically, but it is just to sync the value from the fastcs attribute into the tango device?

It might be nicer to use the attribute update_callback to call a tango device function to update the value when it changes, like the EPICS backend does here.

No, I believe that Tango is only reading that method when the client makes a request and then gives the result to the client. I am not aware that it records the value... According to Tom, such client polling is used at some facilities.

But this is a good point. Tom has mentioned that using Tango events is also popular in the Tango space. Hmm doesnt update_callback update regardless of "change"? If we can make it execute on a change only (i.e. the polled attribute was true and now its false) then we could link the Tango method push_change_event (self, attr_name, data) which can be used for event subscribers. Maybe this is better for a future PR as it will need more investigation and testing?

[marcelldls]

@GDYendell Comments processed, tests fixed. I think this is ready

[GDYendell]

Hmm doesnt update_callback update regardless of "change"?

It does currently still call the callback if the value has not changed, but we could rethink that. softioc ignores sets with the same value and that does cause problems sometimes. A common annoyance is if you make a PV that runs a command when set to 1, then that PV needs to be manually set back to 0 afterwards for the user to be able to run the command again. We would have to be careful not to recreate the same issue at the attribute level. Let's leave it for now.

[marcelldls]

It does currently still call the callback if the value has not changed, but we could rethink that

This would probably also be useful for webhooks

[marcelldls]

@GDYendell Thanks for the additional remarks, I like it. The only thing I am not liking is the enum being "" as default, but I dont feel that is work for this PR

[marcelldls]

At the moment, tango enums are not used here. As is, I believe I will have to find and cast "fastcs enums" into python int enums. Pytango supports Python IntEnum as a datatype. FastAPI supports string and int enum (fastapi/fastapi#2129)

# https://tango-controls.readthedocs.io/projects/pytango/en/stable/api/data_types.html
from enum import IntEnum

from tango.server import Device, attribute

class Noon(IntEnum):
    AM = 0  # DevEnum's must start at 0
    PM = 1  # and increment by 1

class Clock(Device):
    @attribute
    def noon(self) -> Noon:
        time_struct = time.gmtime(time.time())
        return Noon.AM if time_struct.tm_hour < 12 else Noon.PM

Is there a reason we dont have enums as something more python native? Maybe something like

# We have this?
big_enum: AttrR = AttrR(
        Str(),
        allowed_values=["AM", "PM"],
    )
# Should rather we have this?
big_enum: AttrR = AttrR(
        Noon()
    )

[GDYendell]

Is there a reason we dont have enums as something more python native?

I think we should explore this, but it might not be trivial to implement because the fastcs datatypes have specific functionality that configure the control system. For example we are adding units to Int, so the enum version would have to have that too. Perhaps fastcs could provide its own IntEnum that just combines Int and enum.IntEnum?

[marcelldls]

@GDYendell all good?