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

Rename PandA to HDFPandA deriving from CommonPandABlocks and StandardDetector #127

Closed
abbiemery opened this issue Feb 29, 2024 · 6 comments · Fixed by #185
Closed

Rename PandA to HDFPandA deriving from CommonPandABlocks and StandardDetector #127

abbiemery opened this issue Feb 29, 2024 · 6 comments · Fixed by #185
Assignees
Milestone

Comments

@abbiemery
Copy link
Collaborator

abbiemery commented Feb 29, 2024

We want the PandA to be unified under the Standard Detector format. To do this we want to have a HDFPandA which derives from Standard detector and CommonPandABlocks. We want this since PandA can make triggers and use triggers. The CommonPandABlocks provides the interface. The Standard detector provides the behaviour that allows you use these triggers and reads in the larger ecosystem: for example to kickoff, collect (during the time frame from trigger, what have my encoders done).

Depends on #126

@abbiemery abbiemery changed the title Rename PandA to HDFPandA deriving from PandABlocks and StandardDetector Rename PandA to HDFPandA deriving from CommonPandABlocks and StandardDetector Feb 29, 2024
@abbiemery
Copy link
Collaborator Author

abbiemery commented Mar 7, 2024

This makes sense to follow your work @evalott100 so if you have time following yours that would be great. If not we'll roll it over but it's probably best to stack it.

@evalott100
Copy link
Contributor

evalott100 commented Apr 10, 2024

There's a decision I'm trying to make...

Our HDFPandA is going to need take a controller on initialisation:

class HDFPandA(PandA, StandardDetector):
def __init__(
self,
prefix: str,
controller: PandaPcapController,
writer: DetectorWriter,
config_sigs: Sequence[SignalR] = (),
name: str = "",
):

However the controller itself will require blocks which are initialised at HDFPandA.connect time:

await asyncio.gather(self.pcap.arm.set(True))
await wait_for_value(self.pcap.active, True, timeout=1)
return AsyncStatus(wait_for_value(self.pcap.active, False, timeout=None))

I've considered making a new abstract class, subclasses of which will contain a function to fill their attributes with the pvi sub devices after the pvi device has connected:

class ContainsPVIBlocks(ABC):
"""A baseclass used to denote that the class will devices set up during PVI"""
@abstractmethod
def fill_blocks(pvi_device: Device):
"""Add neccessary blocks to attributes from the passed in pvi_device"""

In our case, in the controller we have:

class PandaPcapController(DetectorControl, ContainsPVIBlocks):
pcap: PcapBlock # pcap will be given by the panda post connect

def fill_blocks(self, connected_panda: PandA) -> None:
_check_for_blocks(connected_panda, "pcap")
self.pcap = connected_panda.pcap

and in HDFPandA we have:

async def connect(
self, sim: bool = False, timeout: float = DEFAULT_TIMEOUT
) -> None:
await super().connect(sim=sim, timeout=timeout)
self.controller.fill_blocks(self)

On the one hand this seems quite ugly to me, though I like the idea of having a way to see that a device will be given signals by a pvi device after it has been connected. I think it would be best to have some docs for our modus operandi on connect-time initialised signals.

@coretl , @danielballan , @abbiemery , @callumforrester any ideas?

@coretl
Copy link
Collaborator

coretl commented Apr 10, 2024

This is the discussion we had before about when we populate the pvi generated block. I think the correct thing to do is:

  • Make a create_devices_from_annotations function that makes all the type hinted blocks, populating them with disconnected signals
  • Use this function in PandA init before creating the controller and writer
  • Fill in the signals and shout loudly if they aren't in pvi at connect

E.g.

class CommonPandaBlocks(Device):
    pcap: PcapBlock

class HDFPanda(StandardDetector, CommonPandaBlocks):
    def __init__(self, prefix):
        self._prefix = prefix
        self._backends = create_devices_from_annotations(self, backend_class=PvaSignalBackend)
        # self.pcap is now a Device instance with disconnected Signals
        # DeviceVectors are still empty
        super().__init__(
            controller=PandaPcapController(pcap=self.pcap),
            # same for writer
        )
    async def connect(self, sim, timeout):
        await fill_devices_from_pvi(self, self._backends, self._prefix + "PVI")
        # Now DeviceVectors are filled, and self.pcap has some extra signals that were in pvi but not in type hints

Does that make sense?

As a side note, my PandA captialization is making the class names difficult to type, please can we drop the capitalization of the A in PandA only for class names, like in my example in this post...

@coretl
Copy link
Collaborator

coretl commented Apr 10, 2024

To do this we need to think about how we get a disconnected signal. We can either replace the backend (maybe pass it in at connect?) or we make the backend with the given class, but a blank pv and set that before calling connect. I'm not sure about which is cleaner, so this might be a whiteboarding session to decide.

@evalott100
Copy link
Contributor

evalott100 commented Apr 10, 2024

Why not something like:

T = TypeVar("T", bound=Signal)

class ConnectTimeSignal(Generic[T]):
    def __init__(self, signal: Optional[T] = None):
        self._signal = signal
        self._connected = signal is not None
        
    def connect(self, signal: T):
        self._signal, self._connected = signal, True

    def __call__(self) -> Optional[T]:
        if not self._connected:
            # raise error for trying to access a signal before PVI connection
        return self._signal
        
class PcapBlock(Device):
    active: ConnectTimeSignal[SignalR[bool]]
    arm: ConnectTimeSignal[SignalRW[bool]]

class CommonPandABlocks(Device):
    status: ConnectTimeSignal[SignalRW] # some top level signal, e.g PANDA:STATUS
    pcap: PcapBlock
    
class HDFPanda(StandardDetector, CommonPandaBlocks):
    def __init__(self, prefix):
        create_devices_from_annotations(self)
        # Same as yours except all the signals are initialised with ConnectTimeSignal(signal=None)
        super().__init__(
            controller=PandaPcapController(pcap=self.pcap),
            # same for writer
        )
    async def connect(self, sim, timeout):
        await fill_devices_from_pvi(self, self._prefix + "PVI")
        # We don't need a backends list, the pvaparser will fill the ConnectTimeSignals which PandaPcapController already has a reference to
        # self.pcap has some extra ConnectTimeSignal that were in pvi but not in type hints

This has a couple of benefits in my opinion:
* It's explicit while reading, and at runtime what you're getting from PVI
* You don't have to set up signals which are always going to be disconnected
* top level signals can use the same method: If we use and disconnected PvaSignalBackend, passing in self.status in this example would require that you start replacing attributes in the signal after initialisation and pre connection (this gets messy right?)

@coretl
Copy link
Collaborator

coretl commented Apr 10, 2024

That's an interesting idea, but the major problem I can think about is that the PcapBlock looks different from every other Device, so you have to do await self.pcap.active().get_value() instead of await self.pcap.active.get_value(). I think the additional brackets is enough to mean we shouldn't do this, as it seems to infer you are calling the signal, and it is changing the public API of the Device to support an internal implementation detail...

@callumforrester callumforrester added this to the 0.3 milestone Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
4 participants