DiamondLightSource / SynchWeb

ISPyB web application
http://diamondlightsource.github.io/SynchWeb/
Apache License 2.0
15 stars 31 forks source link

Assumes one-to-one mapping of beamline to PV prefix #53

Closed jlmuir closed 9 months ago

jlmuir commented 5 years ago

In api/config_sample.php, there is a variable $bl_pv_map that assumes a one-to-one mapping between a beamline and a PV prefix:

# Map of beamlinename to pv prefix
$bl_pv_map = array(
  'i02' => 'BL02I',
  'i03' => 'BL03I',
);

At some sites (e.g., IMCA-CAT), there is not a one-to-one between a beamline and a PV prefix, so it would be better to just list PVs explicitly for a given beamline rather than trying to build them dynamically with a PV prefix based on the beamline.

ndg63276 commented 1 year ago

Raised as an internal issue at https://jira.diamond.ac.uk/browse/LIMS-697

ndg63276 commented 10 months ago

@jlmuir I appreciate this ticket is from 4+ years ago, but is this just a hypothetical problem? The $bl_pv_map variable is only used in one (very DLS-specific) place as far as I can see.

jlmuir commented 10 months ago

I didn't investigate how it's used; I was just editing the config to be appropriate for the IMCA-CAT beamline instance, and when I encountered this $bl_pv_map variable, I knew that I couldn't possibly edit its value to be correct for IMCA-CAT because it makes the assumption that a beamline has exactly one PV prefix, which is not the case at IMCA-CAT.

ndg63276 commented 10 months ago

Thanks @jlmuir, good to know it isn't actively breaking things, even if it seems badly written. The same is actually true at Diamond, i03 has PVs starting FE03I (for front end) and also BL03I (for optics and endstation).

We don't really want to list the PVs explicitly, as there are 37 per beamline, all of the format BLxxI-MO-ROBOT-01:PUCK_yy_NAME, where yy is between 01 and 37. The PV is to say which puck is loaded in which position, eg:

BL03I-MO-ROBOT-01:PUCK_01_NAME CPS-0001

Do you have anything equivalent? How are your PV prefixes split, could we just give another key eg

$bl_pv_map = array(
    'i03' => array(
        'frontend' => 'FE03I',
        'optics' => 'BL03I',
        'endstation' => 'BL03I',
    )
);
jlmuir commented 10 months ago

Yes, we have ours split by end station prefix as well (e.g., 17ida: and 17idb:). However, we also have some others such as 17ia: and 17ib:, which were used to make the PVs shorter by 1 character, but these will likely go away. Also, we have some APS-provided PVs, which have their own PV prefixes, and are also not just one prefix (e.g., 17ID:, G:AHU:, and S:). So, if you want SynchWeb to be a portable ISPyB front-end and API that can be deployed at beamlines other than Diamond beamlines, I think it would be good to give up on the prefix approach and just list the PVs in a config somewhere. But if you don't want to do that, then a workaround is that I could create EPICS PVs with the expected prefix that function as aliases for the PVs that don't have the expected prefix. This is kind of like creating a PV compatibility layer. But to do this, SynchWeb should document all the PVs it requires at which PV prefix; otherwise, I'd have to search through the source code trying to find them all.

ndg63276 commented 10 months ago

Ok, have a look at PR https://github.com/DiamondLightSource/SynchWeb/pull/712, does that seem sensible?

jlmuir commented 10 months ago

Unfortunately, I won't have time to review the PR properly in the near future, and it's been a long time since I last looked at this stuff, but after a quick look, yes, I think it seems sensible. Thanks!

I don't know much about PHP, so it could be something obvious, but one thing I didn't understand in the PR was the change to api/config_sample.php: why is there a single quote after the % character in the literal string value?

    # Map of beamlinename to puck name pv
    $bl_puck_names = array(
        'i03' => "BL03I-MO-ROBOT-01:PUCK_%'02d_NAME"
    );
ndg63276 commented 10 months ago

The single quote defines the padding, so '0 means pad with zeroes - https://www.w3schools.com/php/func_string_sprintf.asp.

Although now I read the actual docs - https://www.php.net/manual/en/function.sprintf.php - the 0 flag is enough to say left-pad with zeroes. So I can probably remove the single quote.