Conversation
HITL Evidence:Optimal Crank/Cam: Last Cam Tooth Wrong Position: Used with latest commit from ECU_HITL. |
KshitijKapoor8
left a comment
KshitijKapoor8
left a comment
There was a problem hiding this comment.
From what I could tell, all of the logic and code is sound. I had some performance and interrupt-context concerns.
Might be worth it to consult with someone who knows about engines to go over high level synch logic with before merging.
| volatile double tooth_period_us = 0.0; | ||
|
|
||
| // Engine phase (0 = 0-360, 1 = 360-720). | ||
| bool engine_phase; |
There was a problem hiding this comment.
should be marked volatile, as we use it in both ISR and task
| volatile uint32_t last_cam_time_us = 0; | ||
|
|
||
| // Instantaneous period between crank teeth (in microseconds). | ||
| volatile double tooth_period_us = 0.0; |
There was a problem hiding this comment.
using a 64 bit value on a 32 bit processor in ISR is probably a bad idea, as it's not atomic and can tear mid write
There was a problem hiding this comment.
we should probably just avoid most floating point arithmetic in ISR context, as that could be messy at high RPMs
| inline uint8_t get_cam_delta() { | ||
| return cam_crank_counter - last_cam_crank_counter; | ||
| }; |
There was a problem hiding this comment.
just as a safety thing, if this underflows we would see garbage data that we would not thing is garbage
doing this wrap safe would be:
uint64_t diff = (uint64_t)cam_crank_counter - (uint64_t)last_cam_crank_counter;
return (uint8_t)(diff % NUM_CRANK_TEETH)
| for (;;) { | ||
| // Wait for sync to be acquired by cam tooth interrupts | ||
| if (!syncState.synced) { | ||
| // Sleep briefly to avoid busy-waiting | ||
| osDelay(1); | ||
| continue; | ||
| } | ||
|
|
||
| // We're synced - perform engine control operations | ||
| float current_angle = get_current_engine_angle(); | ||
|
|
||
| // TODO: Schedule fuel injection events | ||
| // TODO: Schedule ignition events | ||
|
|
||
| // ULOG_INFO("EngAngle: %.2f", current_angle); | ||
|
|
||
| if (!syncState.synced) { | ||
| ULOG_WARNING("Lost sync! Re-acquiring..."); | ||
| continue; | ||
| } | ||
|
|
||
| // Sleep briefly - actual timing is interrupt-driven | ||
| osDelay(1); |
There was a problem hiding this comment.
Might be wrong, but I think this is a TOCTOU issue with syncState.synced
| // crank teeth and 3 cam teeth, two opposing one | ||
| // 30deg offset. | ||
| #define NUM_CRANK_TEETH 12 | ||
| #define DEG_BTWN_TEETH (uint8_t)(360 / NUM_CRANK_TEETH) |
There was a problem hiding this comment.
should be kept as float as this is used in float math and would require constant promotions i think
| if (syncState.crank_index == NUM_CRANK_TEETH - 1) { | ||
| syncState.engine_phase = !syncState.engine_phase; |
There was a problem hiding this comment.
Are we sure we can always assume this to be true? what happens if we get shifted ever?
| // If we're synced, validate that the cam delta matches expectations | ||
| if (syncState.synced && syncState.last_cam_crank_counter > 0) { | ||
| // Valid deltas are 2, 10, or 12 teeth between cam pulses | ||
| // Any other delta means we lost sync (missed teeth) | ||
| if (delta != 2 && delta != 10 && delta != 12) { |
| * @param None | ||
| * @retval Fraction of tooth passed (0.0 to 1.0) | ||
| */ | ||
| float get_current_fraction_of_tooth(); |
| * @param None | ||
| * @retval Current engine angle in degrees (0.0 to 720.0) | ||
| */ | ||
| float get_current_engine_angle(); |
| @@ -0,0 +1,362 @@ | |||
| #include "mock_hal.h" | |||
There was a problem hiding this comment.
maybe add a test to verify that the EMA updates to tooth_period_us is applied and correct
2 participants
Addresses issue #3.
SyncStatestruct which contains all information related to cam and crank position.detectSync()to see how our crank and cam line up with this implementation.