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

Remove obsolete edl files #310

Open
MarkRivers opened this issue Jan 28, 2018 · 15 comments
Open

Remove obsolete edl files #310

MarkRivers opened this issue Jan 28, 2018 · 15 comments
Assignees

Comments

@MarkRivers
Copy link
Member

Many of the files in ADApp/op/edl are probably not needed and should be removed. The adl files are can now be automatically converted to edl, ui, and opi by running make in the op/ directory. The edl/autoconvert files are thus in many cases more up to data than the equivalent file in edl/, and the ones in edl/ should thus be removed. I am not an edm expert, and I don't know what files are in use at edm sites (e.g. SLAC and Diamond), so I would like people from those sites to delete the obsolete files.

@ulrikpedersen
Copy link
Member

I think most of those EDM files in edl/ are used by the "DLS screens" (i.e. the 'tabbed' EDM screen layout that @thomascobb did years ago). We do maintain those screens although they rarely change. We can review which ones are used and which can go.

@ulrikpedersen
Copy link
Member

@MJGaughran will take a look at this to identify what we use and what we don't use at DLS :-)

@bhill-slac
Copy link
Contributor

I've been trying to resolve how to deal w/ edl file changes here at SLAC. We started w/ the screens now in ADApp/op/edl years ago and have made many changes since then to make them more consistent w/ our fonts and colors, fix layout issues, etc. I've hesitated to submit these changes, as one change in particular as it isn't backward compatible. We prefix each embedded or related screen reference w/ "areaDetectorScreens/", which allows us to use soft links to locate the desired release of the screens. The workaround isn't hard, it just requires these edm screens to be deployed in a directory named areaDetectorScreens, or to add a soft link w/ that name in your EDMDATAFILES path.
I've just in the last few days collected all our edl changes as a git branch which starts w/ a recent copy of ADApp/op/edl/*.edl in ADApp/op/edl/slac-edl and applies our changes as a series of commits.

I'm thinking the best way forward would be for me to submit this branch as a pull request. Then @MJGaughran can give them a try. I'll also take a look myself at whether or not there are any obsolete files. Then we can decide whether or not to keep the slac-edl variant or merge them w/ the current set.
Cheers, Bruce

@ulrikpedersen
Copy link
Member

I'm also aware that the "DLS" style of EDM screens may not work easily for others as we auto-generate the top-level "tabbed" container screen and also have a 'standard' colour-scheme. So now we have up to 3 sets of somewhat different screens it may be time to separate them in an organised fashion?

In my view there are a couple of ways to do this:

  • Move all site-specific screens out into separate repositories. Keeping only the auto-converted (and tweaked) EDM screens from @MarkRivers in ADCore
  • Move each site-specific set of screens into separate dirs within ADCore. I.e. op/edl/dls nad op/edl/slac

Thoughts?

@bhill-slac
Copy link
Contributor

bhill-slac commented Feb 2, 2018 via email

@MJGaughran
Copy link

We agree with the removal of NDFileCBF and NDFileReduced. Ulrik suggests that simTop is another historical file that can be removed.

NDAttribute(N) and NDROIStat(N) are screens that appear to have never been linked to in their template files, and have a couple of mistakes. I am waiting for someone to let me know if they intend to use these screens; otherwise, they will also be deleted.

I will make a pull request once we have agreed on the edl folder structure, and I have heard back about the above screens.

@MJGaughran
Copy link

NDAttribute and NDROIStat screens are used in a support module, but should be maintained in the ADCore module. I may also make a couple of small changes to them.

The only thing left is to decide on the edl folder structure (separate repositories or separate folders). Any further comments on this?

@bhill-slac
Copy link
Contributor

I'm in favor of keeping the edl sceens in ADCore as we deploy them based on the ADCore release tag.
That way as new features are added to ADCore, the ioc's that use the new ADCore version have
access to the corresponding screens that expose the new features.

@MarkRivers
Copy link
Member Author

Do you also plan to have a slac-edl directory for each detector? I am ambivalent about putting site-specific files in the areaDetector respository.

@bhill-slac
Copy link
Contributor

bhill-slac commented Feb 7, 2018 via email

@ulrikpedersen
Copy link
Member

For us it would definitely be easier to keep the EDM screens in ADCore, as long as they don't get mangled up and conflict with other sites screens ;-). However, in my opinion and from a (areaDetector) maintainer point of view, the site-specific screens really don't belong in ADCore. So on the whole I'm in favour of moving these screens out of ADCore as The Right Thing To Do(tm).

There are a couple of ways that sites like DLS or SLAC can handle this with minimal fuss:

  • Keep a fork of ADCore with a site-specific branch (i.e. we have a "dls-master" branch) and keep site-specific additions there.
    • We have configuration changes and a few other additions there - and could keep our screens there as well.
    • If the fork is kept on github it is relatively easy to maintain and others can grab the screens from there if they wish.
  • Make an entirely new site-specific EPICS module with just the screens.
    • Make it depend on ADCore in order to link it to a specific version.
    • Push it to your github site-specific organisation so that others can use the screens if they wish.

@bhill-slac
Copy link
Contributor

Re a fork w/ a site-specific branch, we're already doing that w/ the slac-edl branch on bhill-slac/ADCore.

Mark, do you know of any other sites that are using edm screens for ADCore, and if so, are they using ADApp/op/edl/*.edl or the autoconvert screens?

I see issues w/ both the existing sets which are why we're maintaining a 3rd set.

Ulrik, I don't see the top-level "tabbed" container screens that you mentioned. Are those generated or maintained outside ADCore?

DLS screens in ADApp/op/edl:

  • These use arial fonts which aren't widely available, so to view them I change the fonts to use a helvetica font that's part of the standard x11 75dpi fonts available on most X11 servers.
  • firefox help launcher requires ADBase.edl to be on user's PATH and be executable

autoconvert screens:

  • Mark fixed some recent filename issues, but the fonts and color schemes from adl2edl aren't very usable. See screenshots below.

slac-edl screens:

  • Requires deployment either in a directory on EDMDATAFILES path named areaDetectorScreens, or a soft link w/ the same name.
  • firefox help launcher requires a directory or soft link in edm's current working dir named areaDetectorHelp.

Screenshot from autoconvert:
adapp-op-edl-autoconvert-ndfiletiff

Screenshot from ADApp/op/edl/NDFileTIFF.edl:
adapp-op-edl-ndfiletiff

Screenshot from slac-edl/NDFileTIFF.edl
adapp-op-edl-slac-edl-ndfiletiff

@MarkRivers
Copy link
Member Author

MarkRivers commented Feb 7, 2018

I would like to suggest that it might be worthwhile to spend some effort on improving the adl2edl converter. In the end if the autoconverted screens can be used directly it can save a lot of effort in manually fixing the edl files every time the adl files change because of new plugin or driver features.

To show that I think this should be feasible I show below the NDPluginStats screen from medm, and the autoconverted versions for CSS-BOY and caQtDM.

This is the medm version.
ndstats_medm

This is the CSS-BOY version (with a 1 character manual fix. Kay has probably already fixed the converter for this).
ndstats_css

This is the caQTDM version.
ndstats_caqtdm

I think you will have to admit that these conversions are very good. I am sure that the edm converter could be made to be just as good.

But here is the autoconverted edm file. I am not an edm expert at all, so there could be things that are not configured correctly in my edm environment.

It looks like the problems are mostly with colors and fonts, which should not be that hard. The related display buttons don't work at all for me.

ndstats_edm

@bhill-slac
Copy link
Contributor

I've been looking into improving adl2edl and have a number of fixes that can greatly improve the autoconverted screens. I'll be pushing those up to epicsdeb/adl2edl.git as I finish them.
The font matching code isn't working at all which is why the fonts look so bad above. Changing the default fonts to more reasonable defaults helps a lot. I'll look further into why font matching isn't working.
There's a -rgb flag that can be used to get the same colors as the medm screen. When -rgb isn't used, the current version just stuffs in the medm color number which isn't useful as the color tables aren't very similar resulting in unreadable text. Introducing default color numbers for key widgets makes the screens look a lot more like the DLS and SLAC color scheme.
The resulting screens are much better, but we'll still use our versions as some hand tweaking is needed for most files, and we've made other changes and improvements.
Mark, I'll leave it up to you whether or not to include the slac-edl screens. If you do I'll push updates as I make them. I think DLS should be the only site to update the ADApp/op/edl/*.edl files since it's
difficult to merge edm files after they've been edited as the widget order in the file can easily change.

@MJGaughran
Copy link

Is there any information on what other sites are using the DLS screens?

Would it be best to wait for the full update to adl2edl, and then remove all of the DLS screens from this repository and keep them on our own branch?

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

No branches or pull requests

6 participants