Please find below my first iteration of comments. I am not through with all code, but would like to start posing questions and raising issues. The points are listed in the sequence I have looked at them.
>> For the Ethernet module the wrong parts number was set in the Siemens "HW-Config" tool: >> the parts number of the CP module actually used in the Pixel PLC system is >> CP 343-1GX21-0XE0 (firmware version v1.0). >> Sorry for that mistake.
>> This has simply been a mistake, which has been fixed now. >> Thank you for bringing this issue to our attention >> (remark: the setting GMT +1 for the NTP time >> will become important once we synchronize the times of the PLC and PVSS systems >> via timer servers.)
>> No, we were simply not aware of this setting. >> We have added IP access protection now. >> Thank you for pointing this out.
>> Added now.
>> Changed now.
>> The activation of diagnostic interrupts has simply not been done. >> The diagnostic interrupts have all been enabled now >> (and OB 82 has been implemented).
>> In case the power is lost (or the PLC simply goes into "STOP" mode), all relays are opened. >> This is the safe state: the opening of relays cause all of the CAEN power supplies to be interlocked.
>> We agree that the warning are potentially dangerous. >> We have shortened the names of variables now, >> so that the code compiles without giving any warnings.
>> We have restructured the Makefile now so that dependencies are accounted for >> in the order by which the functions are compiled.
>> The Data-Blocks DB 6xx actually contain configuration parameters (and operator commands) uploaded by PVSS. >> Concerning the OB 1 cycle-time histogram, we believe that all information that is essential for monitoring the PLC system >> needs to be available in PVSS, not Step7 only. >> (Step7 can only be accessed via a dedicated gateway and is intended for use by PLC experts only.) >> The **distribution** of cycle times can actually not be displayed by Step7 (only the minimum, mean and maximum values can), >> and the amount of code neccessary to fill the histogram is rather small >> (so we will keep the cycle-time histogram).
>> Both the 100ms and the 1.6s are the result of a choice made long ago. It was never intended to be the >> final value. >> >> The Relay is specified for 1 million switching cycles for a current of 0.5 A at a voltage of 24V DC. >> We expect the current to be significantly smaller in the CMS Pixel PLC system, >> so the lifetime of 1 million switching cycles represents a lower limit. >> Assuming 1 million switching cycles as a conservative estimate of the Relay lifetime, >> the Relay might actually break after 2-3 weeks of operation when we toggle the heart-beat >> with a period of 1.6s, as originally intended. >> We agree that this is clearly not acceptable, >> even if we switch channels in case the Relay breaks (each Relay module has 8 channels). >> We will change the heart-beat period to 2 minutes. >> (remark: this is the value used by the Silicon Strip Tracker.) >> This yields a lower limit for the lifetime of the Relay of 3-4 years, >> which we find acceptable. >> Thank you for bringing this issue to everyones attention.
>> The OB 1 cycle time is 250 - 500ms right now. >> The Pixel DCS have ordered a more performant PLC (CPU319) model to replace the current (CPU315) one. >> This upgrade will reduce the cycle time to ~50ms >> (The CPU 319 model is capable of processing 10 times more instructions per unit of time than the current CPU 315 model).
>> The memory consumption is about 115kb for the version of the Pixel PLC code that has been subject of the review. >> >> We have not yet measured which functions consume how much of the cycle time. >> We do not intend to investigate the consumption of cycle time in detail at this time, >> simply because we think it is not an issue for a cycle time of ~50ms >> (after the upgrade of the PLC to a CPU 319 model).
>> Thank you. >> We simply were not aware that ";" represents "no-operation". >> The "dummy" instruction that you refer to has been removed from OB 100 now.
>> The addresses of the hardware push-buttons ("Reset", "Acknowledge" and "Kill") >> are not defined yet. The addresses will be added to DB 1 once they are defined. >> The number of buttons that get processed by the PLC code is set to zero in DB 1, >> so there is actually no problem with the addresses being undefined >> (arrays in which the addresses are strored being unitialized).
>> We prefer to keep constants defined in DB 4 rather than hard-coding their values >> in the SCL source code of different functions. This is a pixel coding requirement.
>> This number defines the size of the array in which the LED status is stored. >> The value is used when iterating over the status LEDs in FC 440 and FC 1013.
>> The organization blocks OB 80, OB 81, OB 82, OB 83, OB 84, OB 85, OB 86, OB 87, OB 88, OB 121 and OB 122 have now been added to the Pixel PLC code. >> (remark: "STOP" is a safe state, so the absence of diagnostic OBs did actually not represent a problem for the safety of the Pixel detector; >> we agree, however, that diagnostic of potential failures is easier in case the diagnostics OBs are implemented.) >> Thank you, Stefan, for bringing this issue to our attention >> and also for providing me (Christian) with some code and taking the time to explain it.
>> The case that the value for the "lower" alarm threshold might be above the value for the "upper" one >> due to an error in the configuration parameters uploaded by PVSS was indeed not properly handled >> in the version of the Pixel PLC code that has been subject of the review. >> This has been fixed now. >> Thank you for bringing this issue to our attention.
>> The checks on the validity of configuration parameters need to be implemented somewhere: >> either in the PLC code or in PVSS. >> Following your proposal, we have thought about moving the configuration parameter checks >> currently implemented in the Pixel PLC code to PVSS and decided not to do it, >> because of the following reasons: >> o it would not reduce the overall amount of code . >> o the PLC code would become simpler, >> but at the same time the PVSS code would become more complex. >> o Stefan, when I (Christian) talked to you about this, >> we came to the conclusion that to some extent it is a matter of programming philosophy and personal preferences >> whether the check of configuration parameters is actually implemented in PVSS or the PLC code. >> o the current implementation of the configuration parameters in the Pixel PLC code has been tested, debugged and verified to work. >> (of course, more tests could still be done, in particular using automated testing procedures, >> if they were available though...) >> The effort to move the code for the configuration parameter checks from Pixel PLC code to PVSS >> and repeat the testing, debugging and verification of the code would be significant. >> It also couples PVSS with the PLC in a complex way. We are not sure this is a good idea at this time.
>> We have added handling of the return codes of the system functions SFC 20 ("BLKMOV") and SFC 21 ("FILL") now. >> Thank you for bringing this issue to our attention.
>> The flexibility of the PLC code to handle possible recablings has actually been adapted from the Silicon Strip Tracker PLC code. >> Right now, it is not clear how often changes to the cabling will occur. >> Following the Silicon Strip Tracker PLC code, we opted to add the flexibility to the Pixel PLC code, >> in order to be prepared for the recabling in case it is neccessary.
>> The function block FC 1110 has been removed from the Pixel PLC code >> (and replaced by "MOD 2", as suggested by Kevin Krause).
>> Other reviewers have expressed a set of different opinions concerning the usefulness of the CRC-16 checksums. >> We intend to bring-up the issue of CRC-16 checksums in the closing discussion of the review.
>> This has been done now. >> Thank you for the suggestion.
>> The purpose of the function block FC 1600 is actually not to verify hardware addresses, >> but to check that the relations between e.g. >> >> UDT610.SensorAddress >> .assocModule_Id and UDT620.Module_baseAddress >> .assocModule_ChannelId >> >> makes sense in order to identify errors in the configuration parameters uploaded by PVSS.
>> We simply did not notice that FC 6666 is an illegal FC number for the CPU 315 PLC model >> when testing the code with the PLC simulator tool. >> The issue with the illegal FC number has been fixed now (changed to FC 666).
>> The precision with which the temperature sensors are calibrated is about 0.5 degrees Celsius. >> The main issue about the "calibration" parameters for temperature sensors is that >> a. Siemens provides the temperature in units of centi-degrees, not degrees, so we need to devide by a factor of 100 >> b. due to space limitations in the inner region of the CMS detector, >> we have to use uncompensated 2-wire read-out over a cable length of about 10m. >> The cable resistance introduces a ~2 degrees shift in the temperature values, >> that we correct for with the calibration parameters as well.
>> The CPU 315 PLC model actually has only 256 timers, not enough for all temperature + humidity sensors.
>> We agree. >> The dew-point can actually never go below -b = -237.7 degrees Celsius, >> so the check is indeed unneccessary and has been removed.
>> Done.* UDT1611: The alarm delay is specified in MS. What are realistic values ? How does this compare to the cycle time ?
>> We do not feel that alarm delays in milli-seconds make sense. >> (We intend to set the alarm delays to values of 2s.) >> The ms is simply the unit of TIME variables in the Siemens S7 PLC system.
>> Yes, we intend to maintain the code in a CVS repository. We have already begun >> implementation.
>> This issue will be addressed by the Pixel Detector and Tracker Management.
>> The Silicon Strip Tracker and Pixel DCS groups considered a joint effort/common DCS/DSS system. >> It turned out, however, that both systems are different when it comes to details, >> and that people worked on different things, at different places and on different time scales.
>> The Pixel Finite State Machine (which is running within PVSS), >> will be integrated into the CMS FSM in the future. >> (The interfaces in terms of FSM states and commands are specified >> in the "CMS DCS Integration Guidelines" document.) >> We hope this answers your question.
>> The power to the Siemens PLC system is provided by a battery backed-up USV, >> so there should be no significant fluctuations in the power supplies.
Final comment: At this moment, I do not consider the code being mature. To me the code is much too complex and focuses too much on auxiliary tasks like exporting monitoring and configuration capabilities to PVSS, or like checking and rechecking data values. I understand the need for high safety, but too complex code will spoil this demand. It should focus on the interlock process and outsource functionality which is not essential. The configuration values can be checked in PVSS, the PVSS S7 Driver should be reliable enough for the data transfer.