Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display BackEMF on Display #371

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions Apps/Inc/UpdateDisplay.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,28 @@ UpdateDisplayError_t UpdateDisplay_SetVelocity(uint32_t mphTenths);
*/
UpdateDisplayError_t UpdateDisplay_SetAccel(uint8_t percent);

/**
* @brief Sets the forward slider value on the display
* @param percent forward power in percent
* @returns UpdateDisplayError_t
*/
UpdateDisplayError_t UpdateDisplay_SetForwardPower(uint8_t percent);

/**
* @brief Sets the regeneration power slider value on the display
* @param percent regenerative power in percent
* @returns UpdateDisplayError_t
*/
UpdateDisplayError_t UpdateDisplay_SetRegenPower(int32_t percent);

/**
* @brief Sets both regeneration and forward power slider value.
* @param forwardPercent
* @param regenPercent
* @returns UpdateDisplayError_t
*/
void UpdateDisplay_SetBackEMF(uint8_t accPercent, int32_t regenPercent);

/**
* @brief Sets the array indicator state on the display
* @param state array contactor on (true) or off (false)
Expand Down
36 changes: 31 additions & 5 deletions Apps/Src/ReadTritium.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@


uint16_t Motor_FaultBitmap = T_NONE;
static float Motor_RPM = CAR_STOPPED; //Car is stopped until velocity is read
static float Motor_Velocity = CAR_STOPPED; //^^^^
static float Motor_RPM = CAR_STOPPED; //Car is stopped until velocity is read
static float Motor_Velocity = CAR_STOPPED; //^^^^
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't do the ^^^^ comment. You can just adjust the first one to say "until velocity and RPM are read"

static float Back_EMF = 0;
static float Bus_Current = 0;
static float Bus_Voltage = 0;

/**
* @brief Returns highest priority tritium error code
Expand Down Expand Up @@ -101,16 +104,39 @@ void Task_ReadTritium(void *p_arg){
Car_Velocity = ((Car_Velocity / 160934) * 10); //Converting from m/h to mph, multiplying by 10 to make value "larger" for displaying

UpdateDisplay_SetVelocity(Car_Velocity);
}

case MC_BUS:{
memcpy(&Bus_Voltage, &dataBuf.data[0], sizeof(float));
memcpy(&Bus_Current, &dataBuf.data[4], sizeof(float));

//Bus voltage is in bytes 0-4
Bus_Voltage = *((float*)(&dataBuf.data[0]));

//Bus Current is in bytes 4-8
Bus_Current = *((float*)(&dataBuf.data[4]));

Comment on lines +110 to +118
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you doing both a memcpy and a pointer dereference to copy the values? Are these not redundant?

Imo, just use the memcpy for code clarity.

int32_t Forward_Power = Bus_Voltage * Bus_Current;

// Divide by maximum power to input percentage
UpdateDisplay_SetForwardPower(Forward_Power / 5000);
}

case BACKEMF:{
memcpy(&Back_EMF, &dataBuf.data[0], sizeof(float)); // BEMFq (The peak of the phase to neutral motor voltage)

//BackEMF is in bytes 0-4
Back_EMF = *((float*)(&dataBuf.data[0]));
Comment on lines +126 to +129
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

int32_t Regen_Power = (Back_EMF * 100) * (Bus_Current * 100); // Fixed point factor (100)

// Divide by maximum power to input percentage
UpdateDisplay_SetRegenPower(Regen_Power / 5000);
}

default:{
break; //for cases not handled currently
}

}


}

OSTimeDlyHMSM(0, 0, 0, 10, OS_OPT_TIME_HMSM_NON_STRICT, &err);
Expand Down
2 changes: 1 addition & 1 deletion Apps/Src/SendTritium.c
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ void Task_SendTritium(void *p_arg){
state.stateHandler(); // do what the current state does
#ifndef SENDTRITIUM_EXPOSE_VARS
readInputs(); // read inputs from the system
UpdateDisplay_SetAccel(accelPedalPercent);
UpdateDisplay_SetBackEMF(accelPedalPercent, brakePedalPercent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason that you only care abt the backemf and not the accel percent? Just curious

#endif
state.stateDecider(); // decide what the next state is

Expand Down
36 changes: 36 additions & 0 deletions Apps/Src/UpdateDisplay.c
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ typedef enum{
// Non-boolean components
VELOCITY,
ACCEL_METER,
REGEN_METER,
SOC,
SUPP_BATT,
CRUISE_ST,
Expand All @@ -55,6 +56,7 @@ const char* compStrings[15]= {
// Non-boolean components
"vel",
"accel",
"regen",
"soc",
"supp",
"cruiseSt",
Expand Down Expand Up @@ -306,6 +308,40 @@ UpdateDisplayError_t UpdateDisplay_SetAccel(uint8_t percent){
return ret;
}

UpdateDisplayError_t UpdateDisplay_SetForwardPower(uint8_t percent) {
static uint8_t lastPercentPower = 0;
if(percent == lastPercentPower){
return UPDATEDISPLAY_ERR_NO_CHANGE;
}

UpdateDisplayError_t ret = UpdateDisplay_SetComponent(MOTOR, percent);
assertUpdateDisplayError(ret);

if(ret == UPDATEDISPLAY_ERR_NONE) lastPercentPower = percent;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity, is this if statement actually necessary? Is this not handled by the assert above this?

return ret;
}

UpdateDisplayError_t UpdateDisplay_SetRegenPower(int32_t percent){
static uint8_t lastPercentRegen = 0;
if(percent == lastPercentRegen){
return UPDATEDISPLAY_ERR_NO_CHANGE;
}

UpdateDisplayError_t ret = UpdateDisplay_SetComponent(REGEN_METER, percent);
assertUpdateDisplayError(ret);

if(ret == UPDATEDISPLAY_ERR_NONE) lastPercentRegen = percent;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above


// In Nextion, we are displaying regeneration slider by
// modifying the background percentage.
return (100 - ret);
}

void UpdateDisplay_SetBackEMF(uint8_t forwardPercent, int32_t regenPercent) {
UpdateDisplay_SetForwardPower(forwardPercent);
UpdateDisplay_SetRegenPower(regenPercent);
}
Comment on lines +340 to +343
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is nitpicky, but if setting backemf is just a wrapper function for simultaneously setting forward and regen power, I'm not sure that's the best idea. I can definitely see someone trying to manually set forward power / regen power and it getting overwritten by a setbackemf call somewhere.

@IshDeshpa any clarity on why we need a dedicated setter for this?

If we do need it, can it maybe me named more specifically, or given a header comment noting that it shouldn't be used in conjunction with the other two calls. Could also deprecate the other two functions if we want everyone to use this moving forward.


UpdateDisplayError_t UpdateDisplay_SetArray(bool state){
static bool lastState = false;
if(state == lastState){
Expand Down
94 changes: 94 additions & 0 deletions Tests/Test_App_DisplayRegen.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// General Imports.
#include "common.h"
#include "config.h"
#include "os.h"
#include "Tasks.h"

// Device specfic imports.
#include "UpdateDisplay.h"
#include "ReadTritium.h"

#define STACK_SIZE 128

static OS_TCB Task1TCB;
static OS_TCB Task2TCB;
static CPU_STK Task1Stk[STACK_SIZE];
static CPU_STK Task2Stk[STACK_SIZE];

int main() {
OS_ERR err;
OSInit(&err);
if (err != OS_ERR_NONE) {
printf("OS error code %d\n", err);
}

// SendTritium
OSTaskCreate(
(OS_TCB*)&SendTritium_TCB,
(CPU_CHAR*)"SendTritium",
(OS_TASK_PTR)Task_SendTritium,
(void*) NULL,
(OS_PRIO)TASK_SEND_TRITIUM_PRIO,
(CPU_STK*)SendTritium_Stk,
(CPU_STK_SIZE)WATERMARK_STACK_LIMIT/10,
(CPU_STK_SIZE)TASK_SEND_TRITIUM_STACK_SIZE,
(OS_MSG_QTY) 0,
(OS_TICK)NULL,
(void*)NULL,
(OS_OPT)(OS_OPT_TASK_STK_CLR),
(OS_ERR*)&err
);

if (err != OS_ERR_NONE) {
printf("SendTritium Task error code %d\n", err);
}

// ReadTritium
OSTaskCreate(
(OS_TCB*)&ReadTritium_TCB,
(CPU_CHAR*)"ReadTritium",
(OS_TASK_PTR)Task_ReadTritium,
(void*) NULL,
(OS_PRIO)TASK_READ_TRITIUM_PRIO,
(CPU_STK*)SendTritium_Stk,
(CPU_STK_SIZE)WATERMARK_STACK_LIMIT/10,
(CPU_STK_SIZE)TASK_READ_TRITIUM_STACK_SIZE,
(OS_MSG_QTY) 0,
(OS_TICK)NULL,
(void*)NULL,
(OS_OPT)(OS_OPT_TASK_STK_CLR),
(OS_ERR*)&err
);
Comment on lines +26 to +61
Copy link
Member

@babusid babusid Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is probably functional, idk if its the best approach. IMO, since the only thing that the UpdateDisplay task relies on is something populating the UpdateDisplay application queue, why not just have a test task written in this file that does that?

That way this task isn't coupled to the other two applications, and can be used freely. Keeping the tests isolated from system changes elsewhere makes them more useful, because what happens if we make some breaking changes to ReadTritium or SendTritium? This test could become useless.


if (err != OS_ERR_NONE) {
printf("ReadTritium Task error code %d\n", err);
}

// UpdateDisplay
OSTaskCreate(
(OS_TCB *)&UpdateDisplay_TCB,
(CPU_CHAR *)"UpdateDisplay_TCB",
(OS_TASK_PTR)Task_UpdateDisplay,
(void *)NULL,
(OS_PRIO)TASK_UPDATE_DISPLAY_PRIO,
(CPU_STK *)UpdateDisplay_Stk,
(CPU_STK_SIZE)DEFAULT_STACK_SIZE / 10,
(CPU_STK_SIZE)DEFAULT_STACK_SIZE,
(OS_MSG_QTY)0,
(OS_TICK)NULL,
(void *)NULL,
(OS_OPT)(OS_OPT_TASK_STK_CLR),
(OS_ERR *)&err);

if (err != OS_ERR_NONE) {
printf("UpdateDisplay Task error code %d\n", err);
}

OSStart(&err);

if (err != OS_ERR_NONE) {
printf("OS error code %d\n", err);
}

return 0;
}