Present:
=========

FNAL
  Eileen Berman (moderator),   Jim Kowalkowski (reviewer). Margaret Votava (scribe)
CERN
 Slawek Tkaczyk (guest), Charles Newsom (guest) , Umesh Joshi (guest), Gennadiy Lukhanin (reviewer),
Christian Veelken (author), Stefan Lueders (reviewer), Piero Verdini (reviewer)
PSI: Lubenski

Overview
==============

In addition to the comments at the meeting, several reviewers have detailed written comments. These 
will
be forwarded to Charles who will make them available to the author and review team. Kevin Krause's
comments will only be in written form and he has since left the laboratory.

Comments are provided on what needs to get done. It's up to developer and the CMS pixel PLC team on 
what
to implement.

Gennadiy expressed interested in supplying comments reviewing the code even further. Charles is happy 
to
get them at any time and
does not have to part of the review.

General comments:
==============

Kevin (reported by Eileen):
 - Return codes from standard and system calls are frequently ignored. This can result in silent
failures. At a minimum record this for diagnostics.

Jim:
 - Too much code to look at for review period. He looked at read channels.
  - Could not get coherent plan on how things are configured. What validation should be done in PLC,
reading functions, etc?
  - Numbering scheme was nice. Easy to connect to the documentation.
  - What are the access rules for a given block? CRC use of multiple flags for determining states. This
can be difficult to determine state space. What
are valid combinations, etc.
  - routines use in/out values. This can be confusing on who is modifying what.
  - how do you validate the system? is there an automatic script. is the code right for this
configuration?

Gennadiy:
  - debugging and unit test information not present. to validate certain blocks.
  - no debugging in the code (no specific debug message)
  - validation - externalize into a separate functional block.
   - spent 8 hours in the code. more detailed functional review later if there is a followup.

Piero:
   - code reproduces some of the siemens tools for diagnostics. this is dangerous.


>> This comment was due to some misunderstanding and has now been resolved, we believe. 


Stephan:
  - Interlock logic
  - Monitoring of PLC should be done in PVSS
  - Communication between PVSS and PLC is too sophisticated.
  - Hardware monitoring should be implemented in OB blocks are not being exploited to do hardware
diagnostics.

Functional groups
============
1) (upload config from PVSS -> PLC)

- Stephan/Jim. THere is a lot of code here. The SCADA system should be checking values, not the PLC. 
PVSS should download, upload, and verify, not do that on the PLC. Put the logic in for access control 
and validation of configuration. Stephan -  In general, PLC has no access rights. That is set in the 
PVSS driver. Can have some IP blocking.

- Gennadiy - would be nice to have a checksum from the database for the configuration information.

- Piero - Sensitivity of PVSS. This goes on separate high speed network for debugging.
PVSS data transmission - strange things to happen from time to time. Packets need to
be broken up on large transmissions. Sometimes blocks are garbled.


>> This would actually be an argument for the CRC16 checksums/
>> validity checks in the PLC of configuration parameters uploaded by PVSS !!


2) reading digital input. (process value)

- Piero - limit floating point values in PLC.
Is there a cost in time for the conversion? (CN), It's a space thing.
Will measure CPU penalty.

3) diagnostics information

- Stefan (and Piero) - should be moved into the OBs where it belongs.

- Piero - remove hardware configuration part. Will cause problems. Only can configure hardware
one way - use the Siemens tools to put it flash since you can't reconfigure it anyway. Want to
only reconfigure when sitting there. Author - we are using Siemens tools.

- Diagnostics OBS bring PLC into a safe state. Cannot download a new module

- Stephan - heartbeat signal. Causes a toggle, which will break down DB6. want a reasonable
value based on MTBF. Point to check.


>> The piece that might break is the Relay module, not DB 6.
>> 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.
>> 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.
>> This yields a lower limit for the lifetime of the Relay of 3-4 years,
>> which we find acceptable.


- Piero, Stephan - Still needs to go to directly Step7 except for
DB630, DB640 DB620,624   Functions: 230, 240, 224 configure PLC from PVSS.
Need to match the hardware address with diagnostic information. Not part of the
hardware configuration. Don't run STEP 7 on the PLC.

4) Interlock conditions open/close relays.

No reviewers reviewed this section. (lack of time)

5) Auxiliary functions

No reviewers reviewed this section. (lack of time)

Any other comments.
====================

Jim: General question. Does Siemens provide a way to run through all possible inputs and them produce
and output table? CN: No.

Missed Gennadiy last comment?

Stephan: Maintenance - how is it archived? Sparing of Modules?
How does it fit into CMS tracker? There are 4 more? Same code, same modules? (CN practically
identical). HW the same. Slawek: all would upgrade together. Unknown about other areas in CMS.

Slawek:
Would like same reviewers for strips. Frank will evaluate if we should do this for all safety systems.
Similarity with DCS

Who gets the detailed comments other than Christian? He then needs to understand implementation
plan. Charles. He will post. Newly generated comments can go directly to Christian and Charles
for posting.

Stehpan: IN a couple of months, followup with the author to see what's been implemented and
why/why not. Authors agree. Do it before Christian leaves. Eileen will organize.

Piero: Can we have list of
common events and responses for next meeting.

Stephan - developer of code is leaving in summer, Transition plan not yet made.