rework FSIO LEelement (#2374)

* save

* firmware

* test parsing multiple things

* split tests

firmware/controllers/core/fsio_core.cpp

@@ -73,7 +73,6 @@ LEElement::LEElement() {
 
 void LEElement::clear() {
 	action = LE_UNDEFINED;
-	next = nullptr;
 	fValue = NAN;
 }
 
@@ -96,7 +95,7 @@ LECalculator::LECalculator() {
 }
 
 void LECalculator::reset() {
-	first = nullptr;
+	m_program = nullptr;
 	stack.reset();
 	currentCalculationLogPosition = 0;
 	memset(calcLogAction, 0, sizeof(calcLogAction));
@@ -104,19 +103,11 @@ void LECalculator::reset() {
 
 void LECalculator::reset(LEElement *element) {
 	reset();
-	add(element);
+	setProgram(element);
 }
 
-void LECalculator::add(LEElement *element) {
+void LECalculator::setProgram(LEElement* program) {
-	if (first == nullptr) {
+	m_program = program;
-		first = element;
-	} else {
-		LEElement *last = first;
-		while (last->next != NULL) {
-			last = last->next;
-		}
-		last->next = element;
-	}
 }
 
 bool float2bool(float v) {
@@ -185,7 +176,7 @@ static FsioResult doBinaryNumeric(le_action_e action, float v1, float v2) {
 /**
  * @return true in case of error, false otherwise
  */
-FsioResult LECalculator::processElement(LEElement *element DECLARE_ENGINE_PARAMETER_SUFFIX) {
+FsioResult LECalculator::processElement(const LEElement *element DECLARE_ENGINE_PARAMETER_SUFFIX) {
 #if EFI_PROD_CODE
 	efiAssert(CUSTOM_ERR_ASSERT, getCurrentRemainingStack() > 64, "FSIO logic", unexpected);
 #endif
@@ -284,7 +275,7 @@ float LECalculator::getValue2(float selfValue, LEElement *fistElementInList DECL
 }
 
 bool LECalculator::isEmpty() const {
-	return first == NULL;
+	return !m_program;
 }
 
 float LECalculator::getValue(float selfValue DECLARE_ENGINE_PARAMETER_SUFFIX) {
@@ -292,12 +283,15 @@ float LECalculator::getValue(float selfValue DECLARE_ENGINE_PARAMETER_SUFFIX) {
 		warning(CUSTOM_NO_FSIO, "no FSIO code");
 		return NAN;
 	}
-	LEElement *element = first;
+
+	const LEElement* element = m_program;
 
 	stack.reset();
 
 	int counter = 0;
-	while (element != NULL) {
+
+	// while not a return statement, execute instructions
+	while (element->action != LE_METHOD_RETURN) {
 		efiAssert(CUSTOM_ERR_ASSERT, counter < 200, "FSIOcount", NAN); // just in case
 
 		if (element->action == LE_METHOD_SELF) {
@@ -312,39 +306,33 @@ float LECalculator::getValue(float selfValue DECLARE_ENGINE_PARAMETER_SUFFIX) {
 
 			push(element->action, result.Value);
 		}
-		element = element->next;
+
+		// Step forward to the next instruction in sequence
+		element++;
 		counter++;
 	}
+
+	// The stack should have exactly one element on it
 	if (stack.size() != 1) {
-		warning(CUSTOM_FSIO_STACK_SIZE, "unexpected FSIO stack size: %d", stack.size());
+		warning(CUSTOM_FSIO_STACK_SIZE, "unexpected FSIO stack size at return: %d", stack.size());
 		return NAN;
 	}
 	return stack.pop();
 }
 
 LEElementPool::LEElementPool(LEElement *pool, int size) {
-	this->pool = pool;
+	this->m_pool = pool;
 	this->size = size;
 	reset();
 }
 
 void LEElementPool::reset() {
-	index = 0;
+	// Next free element is the first one
+	m_nextFree = m_pool;
 }
 
 int LEElementPool::getSize() const {
-	return index;
+	return m_nextFree - m_pool;
-}
-
-LEElement *LEElementPool::next() {
-	if (index >= size) {
-		// todo: this should not be a fatal error, just an error
-		firmwareError(CUSTOM_ERR_FSIO_POOL, "LE_ELEMENT_POOL_SIZE overflow");
-		return NULL;
-	}
-	LEElement *result = &pool[index++];
-	result->clear();
-	return result;
 }
 
 bool isNumeric(const char* line) {
@@ -393,9 +381,9 @@ le_action_e parseAction(const char * line) {
 
 static char parsingBuffer[64];
 
-LEElement *LEElementPool::parseExpression(const char * line) {
+LEElement* LEElementPool::parseExpression(const char * line) {
-	LEElement *first = nullptr;
+	LEElement* expressionHead = m_nextFree;
-	LEElement *last = nullptr;
+	LEElement* n = expressionHead;
 
 	while (true) {
 		line = getNextToken(line, parsingBuffer, sizeof(parsingBuffer));
@@ -404,12 +392,13 @@ LEElement *LEElementPool::parseExpression(const char * line) {
 			/**
 			 * No more tokens in this line, parsing complete!
 			 */
-			return first;
-		}
 
-		LEElement *n = next();
+			// Push a return statement on the end
-		if (!n) {
+			n->init(LE_METHOD_RETURN);
-			return nullptr;
+
+			// The next available element is the one after the return
+			m_nextFree = n + 1;
+			return expressionHead;
 		}
 
 		if (isNumeric(parsingBuffer)) {
@@ -429,17 +418,7 @@ LEElement *LEElementPool::parseExpression(const char * line) {
 			n->init(action);
 		}
 
-		// If this is the first time through, set the first element.
+		n++;
-		if (!first) {
-			first = n;
-		}
-
-		// If not the first, link the list
-		if (last) {
-			last->next = n;
-		}
-
-		last = n;
 	}
 }
 

firmware/controllers/core/fsio_core.h

@@ -58,6 +58,7 @@ typedef enum {
 	LE_METHOD_PPS = 125,
 	LE_METHOD_TIME_SINCE_TRIGGER_EVENT = 127,
 	LE_METHOD_IN_MR_BENCH = 128,
+	LE_METHOD_RETURN = 130,
 
 #include "fsio_enums_generated.def"
 
@@ -103,20 +104,19 @@ public:
 
 	le_action_e action;
 	float fValue;
-
-	LEElement *next;
 };
 
 class LEElementPool {
 public:
 	LEElementPool(LEElement *pool, int size);
-	LEElement *pool;
+
-	LEElement *next();
 	void reset();
 	LEElement * parseExpression(const char * line);
 	int getSize() const;
 private:
-	int index;
+	LEElement* m_pool;
+	LEElement* m_nextFree;
+
 	int size;
 };
 
@@ -132,18 +132,25 @@ public:
 	LECalculator();
 	float getValue(float selfValue DECLARE_ENGINE_PARAMETER_SUFFIX);
 	float getValue2(float selfValue, LEElement *fistElementInList DECLARE_ENGINE_PARAMETER_SUFFIX);
-	void add(LEElement *element);
+
 	bool isEmpty() const;
 	void reset();
 	void reset(LEElement *element);
+
+	// Log history of calculation actions for debugging
 	le_action_e calcLogAction[MAX_CALC_LOG];
 	float calcLogValue[MAX_CALC_LOG];
 	int currentCalculationLogPosition;
+
 private:
+	void setProgram(LEElement* program);
+
 	void push(le_action_e action, float value);
-	FsioResult processElement(LEElement *element DECLARE_ENGINE_PARAMETER_SUFFIX);
+	FsioResult processElement(const LEElement* element DECLARE_ENGINE_PARAMETER_SUFFIX);
 	float pop(le_action_e action);
-	LEElement *first;
+
+	LEElement* m_program = nullptr;
+
 	calc_stack_t stack;
 };
 

firmware/controllers/core/fsio_impl.cpp

@@ -559,9 +559,9 @@ static void showFsio(const char *msg, LEElement *element) {
 #if EFI_PROD_CODE || EFI_SIMULATOR
 	if (msg != NULL)
 		scheduleMsg(logger, "%s:", msg);
-	while (element != NULL) {
+	while (element->action != LE_METHOD_RETURN) {
 		scheduleMsg(logger, "action %d: fValue=%.2f", element->action, element->fValue);
-		element = element->next;
+		element++;
 	}
 	scheduleMsg(logger, "<end>");
 #endif

unit_tests/tests/test_logic_expression.cpp

@@ -44,7 +44,7 @@ FsioResult getEngineValue(le_action_e action DECLARE_ENGINE_PARAMETER_SUFFIX) {
 	}
 }
 
-TEST(fsio, testParsing) {
+TEST(fsio, testTokenizer) {
 	char buffer[64];
 
 	ASSERT_TRUE(strEqualCaseInsensitive("hello", "HELlo"));
@@ -64,29 +64,47 @@ TEST(fsio, testParsing) {
 
 	ASSERT_TRUE(isNumeric("123"));
 	ASSERT_FALSE(isNumeric("a123"));
+}
 
+TEST(fsio, testParsing) {
 	LEElement thepool[TEST_POOL_SIZE];
 	LEElementPool pool(thepool, TEST_POOL_SIZE);
 
-	LEElement *element;
+	LEElement *element = pool.parseExpression("1 3 AND not");
-	element = pool.parseExpression("1 3 AND not");
 	ASSERT_TRUE(element != NULL);
 
-	ASSERT_EQ(element->action, LE_NUMERIC_VALUE);
+	ASSERT_EQ(element[0].action, LE_NUMERIC_VALUE);
-	ASSERT_EQ(element->fValue, 1.0);
+	ASSERT_EQ(element[0].fValue, 1.0);
 
-	element = element->next;
+	ASSERT_EQ(element[1].action, LE_NUMERIC_VALUE);
-	ASSERT_EQ(element->action, LE_NUMERIC_VALUE);
+	ASSERT_EQ(element[1].fValue, 3.0);
-	ASSERT_EQ(element->fValue, 3.0);
 
-	element = element->next;
+	ASSERT_EQ(element[2].action, LE_OPERATOR_AND);
-	ASSERT_EQ(element->action, LE_OPERATOR_AND);
 
-	element = element->next;
+	ASSERT_EQ(element[3].action, LE_OPERATOR_NOT);
-	ASSERT_EQ(element->action, LE_OPERATOR_NOT);
 
-	element = element->next;
+	// last should be a return instruction
-	ASSERT_TRUE(element == NULL);
+	ASSERT_EQ(element[4].action, LE_METHOD_RETURN);
+
+	ASSERT_EQ(pool.getSize(), 5);
+}
+
+TEST(fsio, parsingMultiple) {
+	LEElement poolArr[TEST_POOL_SIZE];
+	LEElementPool pool(poolArr, TEST_POOL_SIZE);
+
+	LEElement* p1 = pool.parseExpression("2");
+	ASSERT_EQ(p1[0].action, LE_NUMERIC_VALUE);
+	ASSERT_EQ(p1[0].fValue, 2);
+	ASSERT_EQ(p1[1].action, LE_METHOD_RETURN);
+
+	LEElement* p2 = pool.parseExpression("4");
+	ASSERT_EQ(p2[0].action, LE_NUMERIC_VALUE);
+	ASSERT_EQ(p2[0].fValue, 4);
+	ASSERT_EQ(p2[1].action, LE_METHOD_RETURN);
+
+	// Check that they got allocated sequentially without overlap
+	ASSERT_EQ(p2 - p1, 2);
 }
 
 static void testExpression2(float selfValue, const char *line, float expected, Engine *engine) {
@@ -201,60 +219,6 @@ TEST(fsio, invalidFunction) {
 }
 
 TEST(fsio, testLogicExpressions) {
-	{
-
-	WITH_ENGINE_TEST_HELPER(FORD_INLINE_6_1995);
-
-	LECalculator c;
-
-	LEElement value1;
-	value1.init(LE_NUMERIC_VALUE, 123.0f);
-	c.add(&value1);
-
-	assertEqualsM("123", 123.0, c.getValue(0 PASS_ENGINE_PARAMETER_SUFFIX));
-
-	LEElement value2;
-	value2.init(LE_NUMERIC_VALUE, 321.0f);
-	c.add(&value2);
-
-	LEElement value3;
-	value3.init(LE_OPERATOR_AND);
-	c.add(&value3);
-	assertEqualsM("123 and 321", 1.0, c.getValue(0 PASS_ENGINE_PARAMETER_SUFFIX));
-
-	/**
-	 * fuel_pump = (time_since_boot < 4 seconds) OR (rpm > 0)
-	 * fuel_pump = time_since_boot 4 less rpm 0 > OR
-	 */
-
-	c.reset();
-
-	LEElement thepool[TEST_POOL_SIZE];
-	LEElementPool pool(thepool, TEST_POOL_SIZE);
-	LEElement *e = pool.next();
-	e->init(LE_METHOD_TIME_SINCE_BOOT);
-
-	e = pool.next();
-	e->init(LE_NUMERIC_VALUE, 4.0f);
-
-	e = pool.next();
-	e->init(LE_OPERATOR_LESS);
-
-	e = pool.next();
-	e->init(LE_METHOD_RPM);
-
-	e = pool.next();
-	e->init(LE_NUMERIC_VALUE, 0.0f);
-
-	e = pool.next();
-	e->init(LE_OPERATOR_MORE);
-
-	e = pool.next();
-	e->init(LE_OPERATOR_OR);
-
-	pool.reset();
-	}
-
 	/**
 	 * fan = (not fan && coolant > 90) OR (fan && coolant > 85)
 	 * fan = fan NOT coolant 90 AND more fan coolant 85 more AND OR