Display BackEMF on Display #371
Conversation
…tium to get BackEMF
…nt32_t by multiplying by 100
…tion that calls both forward and regen power display.
|
Have been working on this with hardware, currently getting Fault screen when flashed on display. Parts that I've modified are the BackEMF power calculation and display functions. |
| 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; //^^^^ |
There was a problem hiding this comment.
don't do the ^^^^ comment. You can just adjust the first one to say "until velocity and RPM are read"
| 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])); | ||
|
|
There was a problem hiding this comment.
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.
| 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])); |
| UpdateDisplay_SetAccel(accelPedalPercent); | ||
| UpdateDisplay_SetBackEMF(accelPedalPercent, brakePedalPercent); |
There was a problem hiding this comment.
is there a reason that you only care abt the backemf and not the accel percent? Just curious
| UpdateDisplayError_t ret = UpdateDisplay_SetComponent(MOTOR, percent); | ||
| assertUpdateDisplayError(ret); | ||
|
|
||
| if(ret == UPDATEDISPLAY_ERR_NONE) lastPercentPower = percent; |
There was a problem hiding this comment.
out of curiosity, is this if statement actually necessary? Is this not handled by the assert above this?
| UpdateDisplayError_t ret = UpdateDisplay_SetComponent(REGEN_METER, percent); | ||
| assertUpdateDisplayError(ret); | ||
|
|
||
| if(ret == UPDATEDISPLAY_ERR_NONE) lastPercentRegen = percent; |
| void UpdateDisplay_SetBackEMF(uint8_t forwardPercent, int32_t regenPercent) { | ||
| UpdateDisplay_SetForwardPower(forwardPercent); | ||
| UpdateDisplay_SetRegenPower(regenPercent); | ||
| } |
There was a problem hiding this comment.
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.
| 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 | ||
| ); |
There was a problem hiding this comment.
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.
Quality Assurance Checklist
To make reviews more efficient, please make sure the software feature meets the following standards and check everything off that meets the quality check. Once everything has been checked, the assigned reviewers will begin the review process. Edit this description to check off the list.
There are exceptions with all guidelines. As long as your decisions are justified, then you are good! Contact the reviewers or the leads about any exceptions.
Requirements
Things to Consider