REF: reorganize, and start AT1K4 (sxr-satt) implementation + simulator

pcdshub / solid-attenuator

LCLS Solid Attenuator System IOC

https://pcdshub.github.io/solid-attenuator

Other

1 stars 4 forks source link

REF: reorganize, and start AT1K4 (sxr-satt) implementation + simulator #63

Closed klauer closed 3 years ago

klauer commented 3 years ago

Description

Reorganize repository to share more code without tying it to a specific IOC.

Motivation and Context

AT2L0 is different than the other 4 solid attenuators to be installed (AT1K4-SOLID being next in line)
Goal: keep PV API as similar as possible, and reuse as much code as possible

The more I get into AT1K4's implementation, the more I'll see if this reorg is a good idea. So, it may shift around significantly by the time it's "ready for review" (though I couldn't really ask anyone to review my mess here in good conscience...)

How Has This Been Tested?

Included unit tests (sadly) only cover some basic calculation things.
Test suite confirms calculations still work
Local, manual testing by way of typhos screens + simulator IOC indicate at2l0-calc remains working

Where Has This Been Documented?

Git history

klauer commented 3 years ago

It's not worthwhile to split up the reorganization and implementation of the SXR attenuator IOCs. This PR will be repurposed for the initial implementation of those, along with its general motion simulator IOC.

klauer commented 3 years ago

~I realize there's now a bit of a mess as to what's a "blade" and what's a "filter" - they pretty well are synonymous in the case of the "in-out filters", but that's not really the case for the ladder variant.~

~In ophyd, the calculator blade 1, filter 1 ends up looking like: calc.filters.filter_01.filters.filter_01 😬 Need to think about this a bit. Maybe even just calc.filter_01.filter_01 would be less bad, removing the DDC... hmm...~ Worked around this a bit, but the naming remains not ideal. ~2021 goal: get better at naming things~ edit: redacted

klauer commented 3 years ago

The "inactive" (do not include in calculations at all) and "stuck in state" (may be stuck out or in) behavior needs some additional work, and special handling in both calculations. This is getting very close to being complete, but I want to ensure these two features work properly before moving this forward.

klauer commented 3 years ago

This is looking better in my local testing. I think both AT2L0 and AT1K4 respect stuck and inactive appropriately.

In the interest of keeping the calculation code relatively simple, the full list of filters and those used in calculations (and those that are stuck) require a confusing and messy set of mappings pre/post calculation. I did what I could to describe line-by-line intent there. It would take a reader a non-trivial amount of time to figure it out even with that.

klauer commented 3 years ago

@ZLLentz, I can't expect a real review here due to the nature of the codebase + size of this PR, but if you have time for even a sniff-test level of checks, it'd be appreciated.

ZLLentz commented 3 years ago

@klauer I'll do my best to sniff through

ZLLentz commented 3 years ago

This PR is absolutely huge

klauer commented 3 years ago

Thanks! And yeah - there was lot more work required here than I had expected.

A small point in its favor is that had this been a traditional soft IOC, I likely wouldn't be halfway done yet...

ZLLentz commented 3 years ago

And had this been PLC code we'd probably be trapped in a nightmare

klauer commented 3 years ago

OK, let's get this out of the way. Follow-up PRs should be shorter. (If not, fire me please)