Ticket 4831 cryosms simple start #2

LilithCole · 2019-11-01T17:02:25Z

Description of work

IOC now performs startup logic

To test

ISISComputingGroup/IBEX#4831

Acceptance criteria

new tests pass

Code Review

Functional Tests

IOC responds correctly in:
- Devsim mode
- Recsim mode
- Real device, if available
Supplementary IOCs (..._0n where n>1) run correctly
Log files do not report undefined macros (serach for macLib: macro to find instances of macLib: macro [macro name] is undefined...

Alistair-McGann-Tessella

This is a very complicated IOC. The structure of the db file is good and it seems to cover most of the funcitonality required well. Most of the changes suggested are minor bug fixes and documentation, which is pretty good given the complexity of the device. 👍

I think it would be useful to add to the wiki page which parameters we expect the users to change, and a brief comment saying that IOC initialisation is done in the asyn driver and why. This will make it clearer to someone supporting this device.

It is worth thinking if any of the PVs should be labelled as interesting and archived.

When I ran the IOC system tests there was quite a lot of errors because of mismatches in the protocols which normally we wouldn't expect to see. There might need to be some work on the emulator and the protocol files to fix these bugs.

Alistair-McGann-Tessella · 2019-11-19T14:33:57Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+	else {
+		const char *magMode = "Non Persistent";
+
+		status = putDb("MAGNET:MODE", &magMode);


I'm not sure what the behaviour will be here, it appears that a string is getting written to a binary (bo) record?

Alistair-McGann-Tessella · 2019-11-19T14:37:28Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+
+	}
+
+	if (std::getenv("USE_SWITCH") == "Yes" && (std::getenv("SWITCH_TEMP_PV") == NULL || std::getenv("SWITCH_HIGH") == NULL || std::getenv("SWITCH_LOW") == NULL ||


The switching variables are different here to the specification documentation. SWITCH_TIMEOUT is missing from this list, and SWITCH_TOLERANCE, SWITCH_TEMP_TOLERANCE, HEATER_OUT have been added?

Alistair-McGann-Tessella · 2019-11-19T15:01:43Z

cryosmsSup/cryosms.proto

+}
+
+setHeaterValue {
+	out "S H %f";


This is different to the design document, which specifies SH.

the design document is incorrect in this instance, command is indeed S H

Alistair-McGann-Tessella · 2019-11-20T10:59:27Z

cryosmsSup/cryosms.db

+record(scalcout, "$(P)OUTPUT")
+{
+    field(DESC, "Output from the PSU in $(DISPLAY_UNIT)")
+    field(FLNK, "$(P)GAUSS")


This is pointing to a PV which does not exist. Is this PV missing or should it be pointing somewhere else?

Alistair-McGann-Tessella · 2019-11-20T11:45:58Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

 	else {
 		return asynSuccess;
 	}
 }

+asynStatus CRYOSMSDriver::onStart()


I think it's important that the logic here is tested so we are aware if we introduce a bug at a later date. I recommend splitting the if statements up into logical chunks and using googletest to unit test each chunk.

Alistair-McGann-Tessella · 2019-11-20T14:12:26Z

cryosmsSup/cryosms.db

+{
+    field(DESC, "Raw value returned from the PSU, A or T")# returned during a “GO” command
+    field(DTYP, "stream")
+    field(INP, "@cryosms.proto getOutput($(P)) $(PORT)")


This protocol also has a mismatch with the emulator, it looks like a CRLF at the end of the return statement. See the devsim logs.

Alistair-McGann-Tessella · 2019-11-20T14:13:01Z

cryosmsSup/cryosms.db

+{
+    field(DESC, "The status of the ramp from the PSU")
+    field(DTYP, "stream")
+    field(INP, "@cryosms.proto getRampStatus $(PORT)")


Mismatches when running against emulator

Alistair-McGann-Tessella · 2019-11-20T16:14:33Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

@@ -95,8 +448,8 @@ extern "C"
 	}
 	// EPICS iocsh shell commands 

-	static const iocshArg initArg0 = { "portName", iocshArgString };			///< A name for the asyn driver instance we will create - used to refer to it from EPICS DB files
-	static const iocshArg initArg1 = { "devicePrefix", iocshArgString };
+	static const iocshArg initArg0 = { "portName", iocshArgString };			///< Port to connect to


👍 Much clearer

Alistair-McGann-Tessella · 2019-11-20T16:39:10Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

 	else {
 		return asynSuccess;
 	}
 }

+asynStatus CRYOSMSDriver::onStart()


When started through the IOC test framework these were the commands issued over asyn (logged using an asyn trace mask). This is a different order to what I was expecting from the specification for the cryo sms:

SET TPA 0.037000 T OFF SET MAX 135.000000 SET RAMP 1.120000 P HEATER GET SIGN T ON T RAMP STATUS GET O GET RATE T

If this is the expected command set on booting with the parameters in the IOC test framework this should be noted in the documentation

Alistair-McGann-Tessella · 2019-11-20T16:51:44Z

cryosmsSup/cryosms.db

@@ -2,15 +2,28 @@
 # Database and stream protocol file for Cryogenic Ltd SMS series superconducting magnet controller commands
 #

-record(bi, "$(P)SIM")


The SIM and DISABLE records are common to all IOCs to make sure that records behave correctly in recsim. Is there a reason that they have been removed?

Alistair-McGann-Tessella

Much tidier now, the bulk of my comments fall in to either documentation (e.g. docstrings for the new functions) and error handling (e.g. adding errors to the IOC log file). The google tests are a really useful tool to keep this maintainable in the future.

CRYOSMSApp/src/CRYOSMSDriver.cpp

Alistair-McGann-Tessella · 2019-12-20T16:23:05Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+
+asynStatus CRYOSMSDriver::checkMaxVolt()
+{
+	asynStatus status;


status is the return value for this function, but I can't see where it's modified here. Perhaps this should be initialised to asynSuccess or similar? This applies to other functions

Alistair-McGann-Tessella · 2019-12-20T16:23:57Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+asynStatus CRYOSMSDriver::checkWriteUnit()
+{
+	asynStatus status;
+	int trueVal = 1;


Can you use c++ booleans instead of defining your own?

In order to set the pvs to their desired states we need to pass a reference to the required integers. I chose to name them trueVal and falseVal purely for readability.

Alistair-McGann-Tessella · 2019-12-20T17:15:40Z

cryosmsSup/cryosms.db

+
+### RAMP:
+
+record(bo, "$(P)RAMP:LEADS")


Does this record need ONAM and ZNAM set to true/false?

Alistair-McGann-Tessella · 2019-12-23T15:05:17Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+	else {
+		status = readFile(envVarMap.at("RAMP_FILE"));
+		if (status != asynSuccess) {
+			this->writeDisabled = TRUE;


Should this condition also have something written to the STAT pv?

CRYOSMSApp/src/CRYOSMSDriver.cpp

Alistair-McGann-Tessella · 2020-01-02T18:38:27Z

CRYOSMSApp/src/tests/CRYOSMSTests.cc

+		ASSERT_EQ(testDriver->writeDisabled, 0);
+	}
+
+	TEST_F(StartupTests, test_GIVEN_use_switch_yes_WHEN_a_required_value_null_THEN_write_disabled)


Ideally, tests like this would be parameterised such that we know that all combinations of NULL/not NULL work for all the environment variables. However, testing one is absolutely good enough, especially given the size of the ticket. I don't suggest these changes unless it's a trivial (<20 minute) job

Alistair-McGann-Tessella · 2020-01-02T18:39:09Z

CRYOSMSApp/src/tests/run_all_tests.cc

+int main(int argc, char **argv) {
+	::testing::InitGoogleTest(&argc, argv);
+	return RUN_ALL_TESTS();
+}


Needs a new line at the end of the file

Alistair-McGann-Tessella · 2020-01-03T10:56:12Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+	pRate_ = (epicsFloat64 *)calloc(INIT_ROW_NUM, sizeof(epicsFloat64));
+	pMaxT_ = (epicsFloat64 *)calloc(INIT_ROW_NUM, sizeof(epicsFloat64));


It's not immediately obvious what these variables do and why they are constructed like this, a comment explaining this would be useful. Becuase they call calloc, it's also important to call free after they are no longer useful to stop a memory leak.

Alistair-McGann-Tessella · 2020-01-03T11:03:57Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+		"USE_MAGNET_TEMP", "MAGNET_TEMP_PV", "MAX_MAGNET_TEMP", "MIN_MAGNET_TEMP", "COMP_OFF_ACT", "NO_OF_COMP", "MIN_NO_OF_COMP_ON", "COMP_1_STAT_PV", "COMP_2_STAT_PV", "RAMP_FILE" };
+	for (std::string envVar : envVarsNames)
+	{
+		envVarMap.insert(std::pair<std::string, const char* >(envVar, std::getenv(envVar.c_str())));


👍 This is a really good way of ensuring that all the environment variables are available here and can be modified by the tests

…ple_start fixed branches

Alistair-McGann-Tessella · 2020-02-12T16:46:41Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+	If not supplied, disables puts and posts relevant status message
+*/
+{
+	asynStatus status;


Initialise to asynSuccess?

Alistair-McGann-Tessella · 2020-02-12T16:47:00Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+{
+/* Checks if the user wants to send data to the PSU in units of amps. Sends this choice to the machine if so, otherwise defaults to tesla.
+*/
+	asynStatus status;


Initialise to asynSuccess

Alistair-McGann-Tessella · 2020-02-12T16:47:12Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+	FAST:ZERO and RAMP:LEADS to 0 and disable them.
+*/
+{
+	asynStatus status;


Alistair-McGann-Tessella · 2020-02-12T16:47:19Z

CRYOSMSApp/src/CRYOSMSDriver.cpp

+	into memory, then read the current field value from the device and send back an appropriate ramp rate based on the ramp table.
+*/
+{
+	asynStatus status;


Alistair-McGann-Tessella · 2020-02-13T15:07:12Z

CRYOSMSApp/src/tests/CRYOSMSTests.cc

+		ASSERT_EQ(testDriver->writeDisabled, 1);
+	}
+
+	TEST_F(StartupTests, test_GIVEN_allow_persist_yes_WHEN_required_values_exist_THEN_write_enabled)


Can there be a test for the case when allow_persist isn't set to yes? Could also be applied to other functions

Alistair-McGann-Tessella · 2020-02-13T15:23:20Z

CRYOSMSApp/src/tests/CRYOSMSTests.cc

+	}
+	TEST_F(StartupTests, test_GIVEN_IOC_WHEN_use_magnet_temp_not_yes_THEN_nothing_happens)
+	{
+		testDriver->envVarMap["USE_MAGNET_TEMP"] = "Yes";


This sets use_magnet_temp to yes?

LilithCole added 5 commits October 28, 2019 09:17

some initial progress

3380658

before I rework the driver to just read env variables

1705c79

attempt 1 at loading macros

a672b30

messily pass tests

5050b88

much tidying

19387b3

pull-request-size bot added the size/XL label Nov 1, 2019

merged from previous ticket, added sim records with script

14a7d92

LilithCole changed the base branch from Ticket651_Add_IOC_CRYOSMS to master November 14, 2019 15:40

Alistair-McGann-Tessella suggested changes Nov 20, 2019

View reviewed changes

LilithCole added 8 commits December 2, 2019 13:29

tidied, ready for tests

1ffce3f

test one runs

adba4ce

most of logic now tested and passing

0d12796

correct commands being sent

c0cf091

mostly finished rework

2694c16

only sim records for reads within current scope of tickets

3d5ff3c

put all the sim records back actually

5342ff8

final tweaks still pass tests

832d9d8

LilithCole mentioned this pull request Dec 9, 2019

HIFI Cryomag: Simple Start Test ISISComputingGroup/IBEX#4831

Closed

LilithCole and others added 4 commits December 13, 2019 11:52

cleaned up a bunch of needless waits, made tests look nicer

bddfd79

typos fixed

93b3c61

Add new line

10cee79

Add new line

0ec9b05

Alistair-McGann-Tessella reviewed Jan 3, 2020

View reviewed changes

LilithCole and others added 3 commits January 21, 2020 09:51

rework

b070245

fix tests

21982c4

Merge pull request #3 from ISISComputingGroup/Ticket_4831_Cryosms_sim…

4a001c8

…ple_start fixed branches

Alistair-McGann-Tessella suggested changes Feb 13, 2020

View reviewed changes

added new tests, initialised status to success

4e76ec0

Alistair-McGann-Tessella approved these changes Feb 14, 2020

View reviewed changes

Merge branch 'master' into Ticket_4831_CRYOSMS_simple_start

3a0fbaf

Alistair-McGann-Tessella merged commit cb96139 into master Feb 14, 2020

FreddieAkeroyd deleted the Ticket_4831_CRYOSMS_simple_start branch September 17, 2024 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ticket 4831 cryosms simple start #2

Ticket 4831 cryosms simple start #2

LilithCole commented Nov 1, 2019

Alistair-McGann-Tessella left a comment

Alistair-McGann-Tessella Nov 19, 2019

Alistair-McGann-Tessella Nov 19, 2019

Alistair-McGann-Tessella Nov 19, 2019

LilithCole Jan 22, 2020

Alistair-McGann-Tessella Nov 20, 2019

Alistair-McGann-Tessella Nov 20, 2019

Alistair-McGann-Tessella Nov 20, 2019

Alistair-McGann-Tessella Nov 20, 2019

Alistair-McGann-Tessella Nov 20, 2019

Alistair-McGann-Tessella Nov 20, 2019

Alistair-McGann-Tessella Nov 20, 2019

Alistair-McGann-Tessella left a comment

Alistair-McGann-Tessella Dec 20, 2019

Alistair-McGann-Tessella Dec 20, 2019

LilithCole Jan 22, 2020

Alistair-McGann-Tessella Dec 20, 2019

Alistair-McGann-Tessella Dec 23, 2019

Alistair-McGann-Tessella Jan 2, 2020

Alistair-McGann-Tessella Jan 2, 2020

Alistair-McGann-Tessella Jan 3, 2020

Alistair-McGann-Tessella Jan 3, 2020

Alistair-McGann-Tessella Feb 12, 2020

Alistair-McGann-Tessella Feb 12, 2020

Alistair-McGann-Tessella Feb 12, 2020

Alistair-McGann-Tessella Feb 12, 2020

Alistair-McGann-Tessella Feb 13, 2020

Alistair-McGann-Tessella Feb 13, 2020


		}

		if (std::getenv("USE_SWITCH") == "Yes" && (std::getenv("SWITCH_TEMP_PV") == NULL \|\| std::getenv("SWITCH_HIGH") == NULL \|\| std::getenv("SWITCH_LOW") == NULL \|\|

		pRate_ = (epicsFloat64 *)calloc(INIT_ROW_NUM, sizeof(epicsFloat64));
		pMaxT_ = (epicsFloat64 *)calloc(INIT_ROW_NUM, sizeof(epicsFloat64));


		### RAMP:

		record(bo, "$(P)RAMP:LEADS")

Ticket 4831 cryosms simple start #2

Ticket 4831 cryosms simple start #2

Conversation

LilithCole commented Nov 1, 2019

Description of work

To test

Acceptance criteria

Code Review

Functional Tests

Alistair-McGann-Tessella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alistair-McGann-Tessella left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment