Gennadiy Lukhanin - PLC Review Comments

To begin with this was my first exposure to PLC code, so for the most part I was looking for typical code elements like debugging information, unit tests, function argument validation and etc.

  1. I haven't found any trace of using code debugging elements. However, from the conversation with Christian I know that there are registers where error numbers can be set and forwarded to PVSS, so a developer can see them. A small example of this technique may be useful to a developer who wants to follow/test the code logic.
    >> Up to now the code has been tested with the PLC simulator tool (PLCSim) provided by Siemens.
    >> This method of testing the code is mainly interactive ("by hand").
    >> The tests that we have done with the PLC simulator tool are described at the end of the Word document. 
    >> The procedures for testing the code in an automatic way are still under investigation.
    >> The lack of automatic testing procedures is actually not specific to the Pixel PLC code
    >> and needs to be addressed in collaboration with other CMS subdetectors.
  2. There are no unit tests of functional tests. Frequently, the initial quality of code is quite high, but subsequent changes and improvements are done quickly and may introduce defects, so these types of tests help developers to assess if their code changes break the existing functionality.
    >> See the previous comments concerning automated testing procedures.
  3. It may be useful to externalize code performing the validation of configuration parameters into a separate functional block, e.g. range validation for configuration settings.
    >> This is in a sense already implemented:
    >> The main purpose of the functions FC1xx **is** to check the validity of configuration parameters uploaded into DB 6xx by PVSS.
    >> Besides checking the configuration parameters, the only other functionality they provide is to transfer the configuration parameters
    >> from the "upload" Data-Blocks DB 6xx to the "work" Data-Blocks DB 1xx.
  4. This comment is more like an improvement suggestion. It may be useful to store checksums for DB10X and DB20X in a database, so the entire chain of data transmission from the database to data blocks can be validated.
    >> Thank you for the suggestion:
    >> We agree that if we keep the check of CRC-16 checksums in the Pixel PLC code
    >> it would be useful to compute the CRC-16 checksums already on the Data-Base level
    >> in order to allow the entire chain of data transmission from the Data-Base to the PLC to be validated.
    >> As other reviewers have expressed a set of different opinions concerning the usefulness of the CRC-16 checksums,
    >> we need to achieve a consensus for the issue of CRC-16 checksums in the closing discussion of the review.