PandABlocks / PandABlocks-ioc

Create an IOC from a PandA
Apache License 2.0
1 stars 5 forks source link

Pass the record prefix into pvi `Signal` components directly #133

Closed evalott100 closed 1 month ago

evalott100 commented 1 month ago

In the new PVI, we no longer pass a prefix here:

https://github.com/PandABlocks/PandABlocks-ioc/blob/6eadee5057eeae38fef75e2ba693d6d8ce17fe53/src/pandablocks_ioc/_pvi.py#L378

Instead we need to pass the whole PV wherever components are created:

https://github.com/PandABlocks/PandABlocks-ioc/blob/6eadee5057eeae38fef75e2ba693d6d8ce17fe53/src/pandablocks_ioc/_pvi.py#L123-L129

We should do the same with softioc. This will involve removing the assignement of a global prefix here:

https://github.com/PandABlocks/PandABlocks-ioc/blob/6eadee5057eeae38fef75e2ba693d6d8ce17fe53/src/pandablocks_ioc/ioc.py#L507-L508

and passing the prefix arg throughout the ioc builder so it can be prepended to the PV strings.

AlexanderWells-diamond commented 1 month ago

If it's helpful, instead of refactoring to not use SetDeviceName, you can retrieve the PV prefix using the GetRecordNames() function:

from softioc.builder import GetRecordNames, SetDeviceName

SetDeviceName("MY-DEVICE-PREFIX")
print(GetRecordNames().prefix)
evalott100 commented 1 month ago

Whoops, accidental close.

If it's helpful, instead of refactoring to not use SetDeviceName, you can retrieve the PV prefix using the GetRecordNames() function:

Ah I was looking for a way to get the record name from whatever softioc record. I still think I'd personally opt for passing the whole PV. I think it's cleaner than getting the prefix of the first record PVI deals with in a loop and appending it to all the widgets.

One thing that I did just think of - we could make the EpicsName class be a dataclass with seperator: str, prefix: str, block: str, field: str | None attributes and then an __str__ which converts them when we pass to things.

AlexanderWells-diamond commented 1 month ago

Yes, there currently exists no official way to extract a PV name from the RecordWrapper interface. Technically it is accessible via <record>._RecordWrapper__device._name, but that's two levels of private interfaces to go through.

If it's a feature that would be useful, raise an enhancement issue against PythonSoftIOC?

But I do see an argument for changing the EpicsName type. It would be a fairly large change given how widely that type is passed around, but it sounds good.

evalott100 commented 1 month ago

If it's a feature that would be useful, raise an enhancement issue against PythonSoftIOC?

I'm unsure if I think it would be useful... On the one hand it could be nice for certain problems, on the other the ioc's datastructures should be designed to aggregate that information anyway. We shouldn't have to give all the initialized records to our PVI wrapper.

But I do see an argument for changing the EpicsName type. It would be a fairly large change given how widely that type is passed around, but it sounds good.

Yep that'd definitely be a seperate issue. But I think design-wise it definitely makes sense to keep prefixes along with the name throughout the codebase

coretl commented 1 month ago

Bear in mind that this is all a temporary fix until we move to FastCS, so fixing it in the easiest way possible will be fine

evalott100 commented 1 month ago

Bear in mind that this is all a temporary fix until we move to FastCS, so fixing it in the easiest way possible will be fine

Ah okay, we'll replace this issue with storing the prefix in the Pvi wrapper class and appending it to all the signals.

Here's the new quick fix.

We add a record_prefix: Optional[str] here:

https://github.com/PandABlocks/PandABlocks-ioc/blob/6eadee5057eeae38fef75e2ba693d6d8ce17fe53/src/pandablocks_ioc/_pvi.py#L237-L241

Then define the prefix for the softioc as well as the pvi stuff, including

Pvi.record_prefix = self._record_prefix

here

https://github.com/PandABlocks/PandABlocks-ioc/blob/6eadee5057eeae38fef75e2ba693d6d8ce17fe53/src/pandablocks_ioc/ioc.py#L507-L508

So that we can prepend Pvi.prefix to all the pvi signal definitions:

https://github.com/PandABlocks/PandABlocks-ioc/blob/6eadee5057eeae38fef75e2ba693d6d8ce17fe53/src/pandablocks_ioc/_pvi.py#L123-L129