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

Motor settings: restructure? #8427

Closed
6 tasks
Tom-Willemsen opened this issue Jul 8, 2024 · 5 comments
Closed
6 tasks

Motor settings: restructure? #8427

Tom-Willemsen opened this issue Jul 8, 2024 · 5 comments
Assignees
Labels
20 Needs pair review Pull request is large or complicated and needs reviewing by a pair of developers

Comments

@Tom-Willemsen
Copy link
Contributor

Tom-Willemsen commented Jul 8, 2024

Where?

Currently motor settings (e.g. jaws.cmd) are sometimes found in a few different places:

  • c:\instrument\apps\EPICS/support/motorExtensions/master/settings
  • c:\instrument\settings\...\galil
    • Or ..\jaws, ..\motorExtensions and other similar directories
  • Sometimes in c:\instrument\apps\epics\support\ioctestframework\master\test_support or test_data and similar directories (for IOC tests)

This can cause problems when settings get out of sync with each other, which recently caused an issue on GEM. See discussion in teams "support-issues" channel 23/04/2024 @ 14:30 for further context.

Probably a brief meeting to decide a plan and then spin off any required tickets

Acceptance criteria

  • Agreed plan about (through either a meeting or online discussion):
    • Exactly which areas are the "authoritative" copies
    • Where the settings are kept so that they can be accessed by both instruments and IOC tests
    • Minimising duplication so that settings in github and in the instrument settings area don't diverge.
    • Whether there are different/better structures for any of these areas that we want to adopt
  • If trivial to implement, implement the plan. If not, spin out tickets

How to Test

verbose instructions for reviewer to test changes
(Add before making a PR)

Time in planning meeting (18/07/24)

1:44:00

@rerpha
Copy link
Contributor

rerpha commented Jul 11, 2024

can we remove c:\instrument\apps\EPICS\support\motorExtensions\master\settings in favour of just making a branch to be merged into whatever instrument they apply to? if they're not used by tests they are probably superfluous?

@Tom-Willemsen
Copy link
Contributor Author

meeting tbd

@Tom-Willemsen
Copy link
Contributor Author

Meeting 14th August @ 15:00

@KathrynBaker KathrynBaker moved this from Backlog to Impeded in PI_2024_02 Jul 30, 2024
@KathrynBaker KathrynBaker moved this to Impeded in PI_2024_08 Aug 2, 2024
@rerpha rerpha added the Needs pair review Pull request is large or complicated and needs reviewing by a pair of developers label Aug 15, 2024
@Tom-Willemsen
Copy link
Contributor Author

Tom-Willemsen commented Aug 15, 2024

Outcome of meeting

  • The central source of truth will be c:\instrument\apps\epics\support\motorExtensions\master\settings\INST_NAME
  • At least initially, the structure below that directory will be equivalent to the existing structure under the settings area
  • Long term view:
    • The settings area should be "numbers" (e.g. motion setpoints files describing mapping of pos name <-> motor SP)
    • The motorExtensions area should be "code" e.g. st.cmd fragments etc.

Acceptance criteria for implementation phase of this issue

  • Existing motor settings in c:\instrument\settings\... for each instrument are copied to c:\instrument\apps\epics\support\motorExtensions\master\settings\INST_NAME
    • Check and adjust any paths as necessary, but do not do any structural refactoring
  • Write an upgrade script for the next deployment that:
    • Runs git log on the motor settings directories
    • If any commits exist in between the date that the settings were copied and the deployment date, raise a message stating that a manual re-copy/re-merge needs to be done
    • If no commits exist in between the date that the settings were copied and the deployment date, delete the motor settings directories in c:\instrument\settings\....
  • Adjust the motor st-common.cmds for all motor controllers to look at motorExtensions\master\settings\INST_NAME rather than the settings area.

@Tom-Willemsen Tom-Willemsen added 20 and removed 3 labels Aug 15, 2024
@Tom-Willemsen Tom-Willemsen moved this from Impeded to Backlog in PI_2024_02 Aug 15, 2024
@Tom-Willemsen
Copy link
Contributor Author

Tom-Willemsen commented Sep 6, 2024

PRs:

Follow-up issue for refactoring:

To review:

  • Check motor settings have been migrated from config area -> motorExtensions
    • Check any hard-coded paths have been adjusted as necessary - but note that structural refactoring was defined as out-of-scope for this issue, so the adjustments should just be things which were "wrong", e.g.:
      • Broken paths
      • Clearly broken code e.g. stuff that had leftover git conflict markers
      • Obvious mistakes in the config area (e.g. instruments defining settings which don't make any sense for that instrument, where the settings have clearly been copied-and-pasted from elsewhere)
    • This is the bit of the change that would be good to pair-review, there are a lot of settings migrated over, I have done my best to go through and check these for any hard-coded paths and any obvious mistakes, but would be good to have several pairs of eyes on this to reduce the chances that I've missed things.
  • Check that the motor IOCs for galil, galilmul, mclennan, linmot, smc100, sm300 and twincat have all been adjusted to point at the new settings area correctly
  • Run a representative selection of ioc system tests using motors and motor settings to prove no obvious functionality has been broken
    • e.g. python run_tests.py -t galil galilmul mclennan linmot refl jaws sm300, and any others which are seen as useful.
  • Check the upgrade script works to remove motor settings from the old place on next deploy, and gives a sensible enough error for a developer to follow if it can't be done automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
20 Needs pair review Pull request is large or complicated and needs reviewing by a pair of developers
Development

No branches or pull requests

7 participants