pcdshub / lcls-twincat-general

A PLC code toolkit for LCLS TwinCAT PLC projects
https://pcdshub.github.io/lcls-twincat-general/
Other
16 stars 20 forks source link

ENH: Thermocouple Pragmas and Generalization #28

Closed ZLLentz closed 4 years ago

ZLLentz commented 4 years ago

FB_ThermoCouple actually supports both thermocouples and RTDs, but the semantics need to be generalized a bit. This PR deprecates the old FB and makes a new FB_TempSensor with the following pragma changes:

Some advice needed: what's the best way to make PREC a configurable field? As it is now it is hard-coded, but if I leave PREC un-pragmad then I expect that a user's outer PREC macro will fill in to the boolean fields as well... Not desired behavior. Is there a way to limit which records the user's outer pragma propagate down to?

klauer commented 4 years ago

Some advice needed: what's the best way to make PREC a configurable field? As it is now it is hard-coded, but if I leave PREC un-pragmad then I expect that a user's outer PREC macro will fill in to the boolean fields as well... Not desired behavior. Is there a way to limit which records the user's outer pragma propagate down to?

I missed these on my first pass, sorry - you can fill in just the precision using a different syntax (see the flight rules https://confluence.slac.stanford.edu/display/PCDS/Beckhoff+Flight+Rules ). But I'm not sure you can have multiple pytmc pragmas in a row, as you'd want the pv prefix to be broadcast to all members and not the precision.

Perhaps we just shrug at the PLC level and pass the buck to EPICS. Turn on autosave for the PREC field, and require a one-time setup step in EPICS.

ZLLentz commented 4 years ago

I'm going to make the two changes discussed above:

  1. Add deprecation warning using the warning pragma
  2. Add default "bad" values to unlinked error variables
ZLLentz commented 4 years ago

@slacAWallace I see you pushed a div by zero fix via github. I'll check to make sure the lib still builds in TwinCAT later. I think the indentations are messed up now.

However, is this the correct way to handle this? Or should this lib have implicit zero div checking?

ZLLentz commented 4 years ago

Also, I'm not convinced that this needs to set a reasonable temperature on zero div because it is the user's mistake to change the scale to zero. There is no case where you want to change this during program execution. There is also no case where the correct value is something other than 10 or 100 as far as I know.

ZLLentz commented 4 years ago

What I'm trying to say is, it's improper to push like this to an open and approved PR. An additional change that needs additional discussion should be treated as second PR.

ZLLentz commented 4 years ago

Ok I want to wrap this up... I made this PR because the pragmas 100% needed to be fixed for some of Maggie's components and it has become too many other things. Also a release was just made and I missed it which makes me extremely sad.

klauer commented 4 years ago

Let's remove the previous commit and make a new tag. It's not too late.

ZLLentz commented 4 years ago

I'm going to fix the spacing and change the default temp to something clearly wrong like absolute zero

ZLLentz commented 4 years ago

Another thought: it may have been better to have a multiplier instead of a divisor, then the default would be 0.1 and there is no possibility of a div by zero

ZLLentz commented 4 years ago

Actually yeah, that would be way more elegant. Any objections to inverting the input parameter?

klauer commented 4 years ago

No real objections given the two scale factors you've shown.

ZLLentz commented 4 years ago

The fResolution parameter is exactly the Resolution parameter from docs like https://www.beckhoff.com/EL3314/, and now there is no zero div handling needed.