Closed ZLLentz closed 1 year ago
Sorry, this definitely broke "something"- I should have slowed down before submitting. My new test passes but I forgot to check the rest of the long test suite.
More detail: something I'm doing has managed to create unexpected bonus records. I'll dive deeper into it soon since this PR is definitely needed prior to deploying the ND motion thing.
What are the new records it finds?
What actually happens here is that the library finds 3 new record candidates in the test case. These are all of type AMSNETID which, has BYTE as its base type. Since we now check base types, and since BYTE is something we could make a record of, it is now found.
Seems like a bug - though I'm curious what our IOC databases will look like after this PR. Any change?
Note that this test makes 3 new records only when allow_no_pragma=True, not for the normal case.
Personally, my database didn't have any weird bonus records other than reinstating the PVs for DUT_MotionStage, etc. I didn't accumulate any AMSNETID records because none of mine are pragma'd (I think?)
Two tests I want to run:
I did test 1 for all of the production beamline plcs that I could quickly remember, provided pytmc master branch ran without errors. This excludes lfe and kfe vac which do not build iocs right now- I will make an issue and investigate separately.
For all the others, this did not change their record generation at all. (Note: no production plc is running the unreleased motion lib that turns DUT_MotionStage into an alias.)
You can snoop on my results at /cds/home/z/zlentz/temp/pytmc_testing/pytmc_testing.diff
and in that folder for my quick/messy metholodoly
Test 2 had more mixed results.
I created an AMSNETID instance in lcls-plc-example-motion and added a pragma (https://github.com/pcdshub/lcls-plc-example-motion/commit/7638617a5e3f0366917ce10839937689bd281892)
Building with pytmc master did not add any PVs pointing to this, which makes sense given what we see in the test suite. Building with this PR finds the symbol and tries to make a record but gets stuck:
My intuition based on the traceback is that the type alias handling I added for DUT_MotionStage actually needs to be done automatically by inspecting the BaseType flag to truly finish the work here. Maybe that belongs in a separate PR though.
I think the "correct" behavior would be:
AMSNETID
to ARRAY[0..5] OF BYTE
because it has a BaseType
element linked to BYTE
and a corresponding ArrayInfo
The bug here is that types aliased from basic types that were previously ignored by pytmc will now make errors instead, which is almost certainly a bug but doesn't seem fatal.
I think in this specific case making AMSNETID
determine that it's just a byte array wouldn't be unreasonable. That being said a more useful conversion would be to support AMSNETID
directly in the IOC where it would auto-convert to a string A.B.C.D.E.F
.
So to that end I'm not sure we want "use the base type" behavior in general... My current feeling is that it's worth going with what you have for starters and revisiting.
You're absolutely right, in this specific case the byte array would be unwieldy. There are other ams net id types that have STRING basetypes though, so I don't think making a special case for the byte array version is the solution either. I think we'd end up pragma-ing one of those and expecting the string to pass through. (e.g. https://github.com/pcdshub/lcls-plc-example-motion/blob/3c9a0681702283bfe79b4211a3f73e063401e2b8/lcls-twincat-motion-example/tc_mot_example/tc_mot_example.tmc#L3526-L3531)
I think what I want to do is:
Without this PR, type aliases don't "see" the pragmas from their type source. This means the database is generated with large numbers of missing records if we keep using the old names (DUT_MotionStage, DUT_PositionState) Database generation diff: https://github.com/pcdshub/lcls-plc-example-motion/pull/10/commits/6bbae6b30127325e0c5d012191152e551f4600b0