Forums

Coding challenge in C for embedded systems

Started by yannoborisho 5 months ago7 replieslatest reply 5 months ago401 views

Hi, 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

[ - ]
Reply by tcfkatJuly 13, 2022

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


[ - ]
Reply by SpiderKennyJuly 13, 2022

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!

[ - ]
Reply by SpiderKennyJuly 13, 2022

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'.

[ - ]
Reply by Bob11July 13, 2022

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.

[ - ]
Reply by yannoborishoJuly 13, 2022

I thought about this. But then I thought the more coding lines I write the smarter you look.

Thanks anyway

[ - ]
Reply by tcfkatJuly 15, 2022

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 ...

[ - ]
Reply by yannoborishoJuly 15, 2022

Thanks. That's a brillant approach. (y)