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

ENH: ND state motion FBs #163

Merged
merged 113 commits into from
Jul 14, 2023
Merged

ENH: ND state motion FBs #163

merged 113 commits into from
Jul 14, 2023

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Mar 7, 2023

Description

This monster is ready for review if anyone is brave enough to take it on. I understand if you don't have time, I don't expect to merge it immediately as it still needs some interactive testing.

This is a really big PR, but most of the line changes are from automatic build updates rather than actual lines of written structured text. Each FB should be as close to single-purpose as possible, so hopefully they are understandable on an individual level.

The main user entry point for the code are the function blocks named e.g. FB_PositionState1D, FB_PositionStatePMPS2D, and the other similarly-named bocks. These have nearly identical input/output APIs and each calls the same "Core" function blocks directly. The core function blocks fan out to include each of the various single-purpose function blocks.

This PR provides new states FBs, as a replacement/upgrade of the previous states FBs, that will let us move multiple motors at once to a multi-dimensional set position coordinated with PMPS with all existing protections. The goal is to do it in a way that is easier to understand and debug and more easily testable with TcUnit than the previous state function block maze, without making it harder to use the state movers on beamline PLCs.

The user still needs to allocate their own pragma'd enum, but the user no longer needs specific subclass incantations and instead can use the entry point function blocks and pass their enum as VAR_IN_OUT. For the multi-dimensional FBs, the user will provide one ST_PositionState array per motor, and otherwise the experience is nearly identical.

The function blocks here are implemented up to 3D, but there's no reason why they can't be trivially expanded to 4D, 5D, ... etc., as needed.

The PMPS blocks now support dynamic parameter changes, e.g. you can completely switch out the database lookup key in your program and expect the new state to load in and be available promptly. You can also disable the various bits of the PMPS handling in code if the requirements demand it (e.g. if a specific protecting device is present).

The underlying composite function blocks pre-allocate an array of N motors rather than using variable length arrays. This is entirely because the TwinCAT compiler was very unhappy with multi-dimensional nested function blocks of variable length, and doing it this way instead made it happier.

Full set of included changes, in the order they are encountered in the diff:

  • Add "Test suite passes locally" to the pull request checklist.
  • Exclude submodule from github action style checks, as this was causing useless noise in the CI output.
  • The tmc file is being ignored, because it is not used at all when installed as a library.
  • The readme has been updated with information about how to use the states function blocks.
  • Data structures prefixed with DUT_ (and one unprefixed struct) have been renamed to ST_ In line with theTwinCAT variable naming section in our style guide. The old names are moved to a "Deprecated" subfolder and are now "obsolete" aliases of the new names, so old code will continue to work albeit with a compiler warning.
  • ProductVersion has been removed from all the xml headers via project setting to reduce diff spam.
  • State EPICS interfaces have been largely consolidated into a few structs instead of distributed among individual variables in the state function blocks.
  • nMaxStateMotorCount and nMaxStates have been added as global variables on MOTION_GVL to keep track of the highest dimension and highest number of states used in the PLC, in case we want to tune the state count or motor count down.
  • MotionConstants.MAX_STATE_MOTORS has been added as a configurable global constant.
  • Old state FBs have been moved to a "Deprecated" subfolder and are now marked as "obsolete".
  • The "PMPS" subfolder has been filled with individual subcomponent function blocks related to motion PMPS that are independently unit testable, as well as the user-facing e.g. FB_PositionStatePMPS2D.
  • New usable examples have been added to the "States/Examples" subfolder.
  • The "States" subfolder has been filled with individual subcomponent function blocks related to states motion that are independently unit testable, as well as the user-facing e.g. FB_PositionState2D.
  • The Interactive program has been supplanted by PRG_TEST which runs the unit tests.
  • Unit tests have been written to cover all states code.
  • 20 test Axes have been added that are used in the test suite
  • Unused solution elements have been hidden from view.
  • The project twincat version has been pinned at 3.1.4024.35 so it will automatically be opened at that version. This is the correct version for running the unit tests interactively on plc-tst-motion.

Motivation and Context

  • It's not possible to implement the TMO FZP protections using the existing infrastructure.
  • In general, having multi-dimensional states will be useful.
  • The older state code is functional but hard to debug and impossible to unit test.

How Has This Been Tested?

Lots and lots of unit tests
I think this needs some trial runs with the common component library and in TMO with the FZP before I can consider it fully tested. I mainly want to make sure that the fast faults are generated fully, and that the PV structure for the 1D function blocks did not change.

Where Has This Been Documented?

I wrote an extensive readme, but probably there is more to write and some of that readme should make it onto the confluence documention.

Pre-merge checklist

  • Code works interactively
  • Test suite passes locally
  • Code contains descriptive comments
  • Libraries are set to Always Newest version (Library, *)
  • Committed with pre-commit or ran pre-commit run --all-files

@ZLLentz
Copy link
Member Author

ZLLentz commented Mar 29, 2023

Some notes about my struggles today:

  • The repo at 477853b is unable to compile with one of those really big "abort/retry/continue" messages.
  • The repo at ebf2ce5 compiles (I switched a bunch of dynamically sized * arrays to static size) but encounters a stack overflow in the test suite. I'm not sure if this stack overflow is because I switched a few things from VAR_IN_OUT to VAR_INPUT (did not need to be modified, no longer required VAR_IN_OUT due to static sizing) and this could have added unnecessary large array memcopies instead of reference passing (?) or if it stack overflows due to the way I've structured an 125 assert unit test.

@ZLLentz
Copy link
Member Author

ZLLentz commented Jul 13, 2023

I've realized that it will not be practical to do any further review on this PR, despite there being more items to address

Instead, I will do the following:

  • Swap the project name back to lcls-twincat-motion from lcls-twincat-motion-zlentz
  • Create an issue for each of the remaining suggestions
  • Create an issue for each of the remaining minor bugs I found
  • Merge
  • Work on these issues in one or more follow-up PRs

@ZLLentz
Copy link
Member Author

ZLLentz commented Jul 14, 2023

I've created a bunch of follow-up items. Some of them are easy, some of them are urgent and need to be done before a tag, and some of them are neither.

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

Successfully merging this pull request may close these issues.

4 participants