ENH: check PLC OS to set new default dir for BSD PLCs #215
Conversation
|
I will run the unit tests right now, but I won't test the Windows CE PLC until Monday. When I'm in the office it will be more straightforward to avoid conflicts. If it works on Windows CE then I'll be confident to mark as ready for review and ask for reviewers next week. |
|
This doesn't brick the test suite, but also I didn't expand the test suite here because I wasn't sure if there was anything meaningful to do. The only test I can think of is a sort of "reflexive" test where I duplicate the logic in the test suite and make sure its being followed but this doesn't have very much value in my opinion. |
|
I think this is done, looking for opinions on the direction + code review + suggestions on some sort of useful unit test I can add |
| eParameterKey:=E_IPCDiag_ParameterKey.OS_Name, | ||
| fbRegister:=fbIPCReg, | ||
| ); | ||
| IF fbCheckOS.bValid THEN |
There was a problem hiding this comment.
I noticed this includes a check for error, but if there is an error, we won't be flagged.
There was a problem hiding this comment.
Good catch. You're right, as-written if we have an error the JSON file will silently never try to load and we might not notice an issue until we check the PMPS diagnostic.
This warrants some revision.
There was a problem hiding this comment.
Some options that I'm tossing around right now:
- If there's an error, try again? (a few times?)
- Go back to the old default?
- Give an error log message?
- Fast fault?
Needs some thought
There was a problem hiding this comment.
I'm going to opt for 3 tries followed by going back to the old default
This gives us some wiggle room for first-cycle-bs and will give us some normal error messages if the old default was wrong and if the old default was correct then the PLC will load values as normal.
There was a problem hiding this comment.
I think this means, if the OS isn't solved and a twincat BSD PLC gets the default directory, fbPmpsFileReader will throw a directory not found error.
|
I'm going to click update branch and re-request reviews here, I forgot about this PR until I checked my jira tickets. |
Description
Use the IPCDiag FBs to check the PLC's OS, then use that OS to decide the default directory for pmpsdb reading.
These function blocks are somewhat strange, but I followed the docs closely and arrived at this implementation.
Here, I've set the new default directory for TwinCAT BSD PLCs to match the associated PRs:
pcdshub/twincat-bsd-ansible#19
pcdshub/pmpsdb_client#25
And I've left the default directory for other PLC types unchanged.
Motivation and Context
We can explicitly set a directory for the PMPS database loading in each project, but this leaves open the possibility for typos and copy/paste errors. The directory should be the same every time (dependent on OS), so we can directly check the OS to set a reasonable default. The code is skipped entirely if the user provides a directory.
How Has This Been Tested?
Interactively on a BSD PLC: the correct directory was checked and the file was opened. (plc-tst-bsd2)
Also checked on a Windows CE PLC, test suite passed and default directory was ftp (plc-tst-proto8, plc-tst-pmps-subsytem-b)
Tested again after adding some error handling, seemed to do the right thing
Where Has This Been Documented?
https://confluence.slac.stanford.edu/display/PCDS/TcBSD+Configuration+Reference#TcBSDConfigurationReference-PMPSDatabase
Pre-merge checklist
Always Newestversion (Library, *)pre-commitor ranpre-commit run --all-files