Coding challenge in C for embedded systems
Started by 2 years ago●7 replies●latest reply 2 years ago●1354 viewsHi, I am taking a coding challenge for a position of embedded software engineer.
Just wondering how you guys would approach this problem described in the zip file Code_Test.zip
Here is my solution :
/**
* This file contains functions to..
*
*/
#include "hwio.h"
#include <stdio.h>
#include <stdlib.h>
/* Defines and types ---------------------------------- */
#define MEASURING_RANGE 40 //Initialisation of the sensor measuring range
#define VOLTAGE_RANGE 10 //Initialisation of the sensor voltage range
#define ACTUATOR_LEFT_END 3 //Initialisation of the actuator left end stop
#define ACTUATOR_RIGHT_END 35 //Initialisation of the actuator left end stop
#define ACTUATOR_STROKE 32 //Initialisation of the actuator stroke
typedef char* byte ;
/* Global variables ----------------------------------- */
byte requestedPositionPercent;
/* Static variables ----------------------------------- */
static T_VOLT_FLT V_sensor_position;
static tActuatorDirection pDir;
static tActuatorEnable pEn;
static int counterLeftEnd = 0; //Left end stop counter
static int counterRightEnd = 0; //Right end stop counter
/**
* Application loop called every 10ms
*/
void applicationLoop10ms()
{
/* Close loop control------- */
float reqPosmm = 0; //Requested position in mm
float feedbackPosSen = 0; //Feedback position by sensor in mm
requestedPositionPercent = getRequestedPosition(); //Acquisition of the setpoint position
V_sensor_position = hwioGetSensorVoltage(); //Acquisition of the current position of piston
//Conversion of V_sensor_position in milimeter
feedbackPosSen = V_sensor_position * ((float)MEASURING_RANGE)/((float)VOLTAGE_RANGE);
//Conversion of the requested position in mm
reqPosmm = (float) ACTUATOR_LEFT_END + ((float)(atoi(requestedPositionPercent)) * (float) ACTUATOR_STROKE/100);
if (reqPosmm > feedbackPosSen) //RequestedPositionPercent higher than SensorPosition
{
pEn = ACTUATOR_ON;
pDir = PISTON_EXTEND;
hwioSetActuatorEnable(pEn);
hwioSetActuatorDirection(pDir);
}
else if (reqPosmm < feedbackPosSen) //RequestedPositionPercent lower than SensorPosition
{
pEn = ACTUATOR_ON;
pDir = PISTON_RETRACT;
hwioSetActuatorEnable(pEn);
hwioSetActuatorDirection(pDir);
}
else if (reqPosmm == feedbackPosSen) //Actuator at the desired setpoint
{
pEn = ACTUATOR_OFF;
hwioSetActuatorEnable(pEn);
}
/* Monitoring of long press against the end stops -------- */
if (feedbackPosSen != (float) ACTUATOR_LEFT_END & feedbackPosSen !=(float) ACTUATOR_RIGHT_END)
{
counterLeftEnd = 0; //Re-init to zero if the actuator is not at its end stop
counterRightEnd = 0; //Re-init to zero if the actuator is not at its end stop
}
else if(feedbackPosSen == (float) ACTUATOR_RIGHT_END) //Actuator at the right end stop
{
if (counterLeftEnd != 0) //Check if the counter has been reinitialised
{
counterLeftEnd = 0;
counterRightEnd = 0;
}
counterRightEnd++; //Increment each 10ms if the actuator is at its end stop
if (counterRightEnd == 300) //Actuator forced against the end stop for 3s
{
pEn = ACTUATOR_ON;
pDir = PISTON_RETRACT;
hwioSetActuatorEnable(pEn);
hwioSetActuatorDirection(pDir);
pEn = ACTUATOR_OFF;
hwioSetActuatorEnable(pEn);
}
}
else if (feedbackPosSen == (float) ACTUATOR_LEFT_END) //Actuator at its left end stop
{
if (counterRightEnd != 0) //Check if the counter has been reinitialised
{
counterLeftEnd = 0;
counterRightEnd = 0;
}
counterLeftEnd++; //Increment each 10ms if the actuator is at its end stop
if (counterLeftEnd == 300) //Actuator forced against the end stop for 3s
{
pEn = ACTUATOR_ON;
pDir = PISTON_EXTEND;
hwioSetActuatorEnable(pEn);
hwioSetActuatorDirection(pDir);
pEn = ACTUATOR_OFF;
hwioSetActuatorEnable(pEn);
}
}
}
//Thanks for your tips
Hello!
1) After all, who is applying, you, or the forum? This is your first post, what have you contributed to the forum?
2) There are number of beginner bugs in this code.
- comparing floats like if(f1 == f2) is useless, this will rarely work. As the setpoint is integer, I personally would do the whole calculation in integer.
- at the end stops, you use the full timeout of 3 seconds, why? When the piston is at the end positions it should be stopped immediatley.
- you define byte requestedPositionPercent; in global vars, this is a Null pointer and your program crashes.
- compiling with gcc -c -Wall -Wpedantic -Wextra solution.c results in:
solution.c: In function ‘applicationLoop10ms’:
solution.c:44:26: warning: assignment to ‘byte’ {aka ‘char *’} from ‘char’ makes pointer from integer without a cast [-Wint-conversion]
44 | requestedPositionPercent = getRequestedPosition(); //Acquisition of the setpoint position
| ^
solution.c:85:20: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses]
85 | if (feedbackPosSen != (float) ACTUATOR_LEFT_END & feedbackPosSen !=(float) ACTUATOR_RIGHT_END)
| ^
You bitwise AND with a single &! You intended logical AND, so use &&
Again, comparing floats is useless.
So, better do NOT apply with this code. Rewrite from scratch.
Regards,
Eric
You were so kind to give the OP such a detailed answer! Of course, anyone else applying for the same thing, and a wee knowledge of Google can now see the OPs solution, and some comments on how to improve it.
I see what you did there!
scnr ...
/**
* (C) tcfkat 2022 for embeddedrelated.com
* NO PERMISSION TO USE EXCEPT FOR EDUCATIONAL USE
*/
#include <stdint.h>
#include <math.h> // for round()
#include "hwio.h"
/* Defines and types ---------------------------------- */
#define TIMEOUT (300) // 3 seconds / 0.01
#define SENSORTRAVEL (40.0f) // Sensor SMAT-8M measuring range (40mm)
#define PISTONTRAVEL (32.0f) // Piston travel range (32mm)
#define SENSORMINVOLTAGE (0.0f) // 0 Volt
#define SENSORMAXVOLTAGE (10.0f) // 10 Volt
#define SENSORSCALE ((SENSORMAXVOLTAGE-SENSORMINVOLTAGE)/SENSORTRAVEL) // here: 0.25V/mm
#define DEADBAND ((SENSORTRAVEL-PISTONTRAVEL)/2.0f) // here: 4mm
#define SENSORLOVOLTAGE (SENSORMINVOLTAGE+(DEADBAND*SENSORSCALE)) // here: 1.0 Volt
#define SENSORHIVOLTAGE (SENSORMAXVOLTAGE-(DEADBAND*SENSORSCALE)) // here: 9.0 Volt
typedef enum
{
PISTON_STOP = 0,
PISTON_OUT,
PISTON_IN
} tActuatorAction;
/* Function prototypes -------------------------------- */
static void actuatorAction(tActuatorAction act)
{
static uint8_t last = PISTON_STOP;
static uint16_t timeout = 0;
if(last != act){ // act change (must not necessarily pass over PISTON_STOP!)
last = act;
timeout = 0;
}else{ // no act change. Check timeout only if piston is moving
if(act != PISTON_STOP){
if(timeout < TIMEOUT)
timeout++;
else
hwioSetActuatorEnable(ACTUATOR_OFF); // piston does not reach final position
}
return; // no act change, do not call hwioSetActuatorxxxx() over and over again
}
switch(act){
case PISTON_OUT:
hwioSetActuatorDirection(PISTON_EXTEND);
hwioSetActuatorEnable(ACTUATOR_ON);
break;
case PISTON_IN:
hwioSetActuatorDirection(PISTON_RETRACT);
hwioSetActuatorEnable(ACTUATOR_ON);
break;
default: // PISTON_STOP or wrong act parameter
hwioSetActuatorEnable(ACTUATOR_OFF);
break;
}
return; // make (some) compiler happy
}
/* External variables and functions ------------------- */
/* Global variables ----------------------------------- */
char requestedPositionPercent;
/* Static variables ----------------------------------- */
/**
* Application loop called every 10ms
*/
void applicationLoop10ms()
{
T_VOLT_FLT SensorVoltage = hwioGetSensorVoltage();
uint8_t actpos;
if ((SensorVoltage < SENSORLOVOLTAGE) || (SensorVoltage > SENSORHIVOLTAGE)){
actuatorAction(PISTON_STOP); // Sensor broken, stop piston immediately!
}else{
requestedPositionPercent = getRequestedPosition();
// Do some bounds checks on param
if((requestedPositionPercent >= 0) && (requestedPositionPercent <= 100)){
actpos = (uint8_t) round((SensorVoltage - SENSORLOVOLTAGE) /
(SENSORHIVOLTAGE - SENSORLOVOLTAGE) *
100.0f);
if(actpos < requestedPositionPercent)
actuatorAction(PISTON_OUT);
else if(actpos > requestedPositionPercent)
actuatorAction(PISTON_IN);
else
actuatorAction(PISTON_STOP); // setpoint reached
}
}
return; // make (some) compiler happy
}
shorter & safer ...
Thanks. That's a brillant approach. (y)
In terms of atual hardware control...
You have a static variable to store the state and direction of the actuator, but you don;t use them, just writing to them never reading. Take this bit of your code for example:
pEn = ACTUATOR_ON;
pDir = PISTON_EXTEND;
hwioSetActuatorEnable(pEn);
hwioSetActuatorDirection(pDir);
You enable the actuator before you set it's direction. It will probably not like that! You should consider it's current state - why bother changing it if it's already doing what you want?
And you should certainly Disable the actuator before ever changing it's direction. Then, after the direction is set, re-enable it. Take into account also the number of times the specification tells you that various things (ie more than 1!) are 'fragile'.
This isn't a detailed answer, but a suggestion: consider using a state machine approach for problems like this. Enumerate the possible states of the actuator and use a switch/case statement to drive the hwio. The resulting code is usually far easier to read, understand and debug than long chains of if/else logic.
I thought about this. But then I thought the more coding lines I write the smarter you look.
Thanks anyway
