Enable correct RPM calculation

Hurricos commented 9 months ago

The two commits in this PR enable correct RPM calculation, which was not working before.

fix: rpm: Count QTY3 ignition pulses per QTY2 revolutions
fix: rpm: Enable pin-change IRQs on port B

Tested-by: Martin Kennedy hurricos@gmail.com

Addenda

I should probably write up a more complete testing procedure so I can document an A/B test.

Hurricos commented 7 months ago

Here is my complete A/B test of each of these commits:

Test procedure for e69d832ce08c7b72398149d7e28ec54ebe8b23c4 (RPM fixup pull request):

git checkout e69d83
Compiled and sent firmware
In ASM, $DISP=OEM to start seeing OEM values, including RPM
Insert key on fully cold engine

Start car, note output:

21:31:43.389 -> MAMODE2:Reg/Idle, MAMODE1:Prestart, CLUTCH:Pressed,  BRAKE:Released, CMDPWR:49% TPS:7% MAP:55% RPM:0
21:31:43.655 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Pressed,  BRAKE:Released, CMDPWR:49% TPS:8% MAP:52% RPM:0
21:31:43.922 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:38% RPM:1034
21:31:44.155 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:27% RPM:1336
21:31:44.421 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:22% RPM:1751
21:31:44.654 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:19% RPM:1801
21:31:44.920 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:7% MAP:18% RPM:1727
21:31:45.153 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:19% RPM:1650
21:31:45.419 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:19% RPM:1577
21:31:45.652 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:20% RPM:1581

git checkout 1c6cfe9 (the commit before: this includes the change-request pin enablement fixup, but not the conversion from pulse to RPM):
Compiled and sent firmware
In ASM, $DISP=OEM to start seeing OEM values, including RPM
Insert key on fully cold engine

Start car, note output:

21:32:26.377 -> MAMODE2:Assist,   MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:55% RPM:0
21:32:26.643 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:43% RPM:897
21:32:26.876 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:26% RPM:1969
21:32:27.142 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:21% RPM:2373
21:32:27.374 -> MAMODE2:Reg/Idle, MAMODE1:AutoStop, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:7% MAP:19% RPM:2535
21:32:27.641 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:19% RPM:2411
21:32:27.907 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:19% RPM:2405
21:32:28.140 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:20% RPM:2397
21:32:28.405 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:20% RPM:2404

My engine was for sure idling at 1600RPM cold. To test 1c6cfe9 ("fix: rpm: Enable pin-change IRQs on port B") to make sure enabling pin-change interrupts was needed, I did:

git checkout 6d2b793a2ac06ecc8f13460337834970078e7dd2 (the current main branch on doppelhub/MuddersMIMA.git):
Compiled and sent firmware
In ASM, $DISP=OEM to start seeing OEM values, including RPM
Insert key on fully cold engine

Start car, note output:

21:39:36.841 -> MAMODE2:Reg/Idle, MAMODE1:Prestart, CLUTCH:Pressed,  BRAKE:Released, CMDPWR:49% TPS:8% MAP:55% RPM:0
21:39:37.074 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Pressed,  BRAKE:Released, CMDPWR:49% TPS:8% MAP:49% RPM:0
21:39:37.340 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:31% RPM:0
21:39:37.573 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:23% RPM:0
21:39:37.839 -> MAMODE2:Assist,   MAMODE1:Starting, CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:18% RPM:0
21:39:38.072 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:19% RPM:0
21:39:38.338 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:20% RPM:0
21:39:38.570 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:19% RPM:0
21:39:38.836 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:19% RPM:0
21:39:39.069 -> MAMODE2:Reg/Idle, MAMODE1:Standby,  CLUTCH:Released, BRAKE:Released, CMDPWR:49% TPS:8% MAP:19% RPM:0

Hurricos commented 3 months ago

Can you optimize the "2/3" math, so the preprocessor only has to perform one division operation? Ideally '2' & '3' are both #define statements, too, so that they aren't magic numbers. Proposed implementation: "(ONE_MINUTE_IN_MICROSECONDS * ENGINE_REVOLUTIONS / NUM_TACHOMETER_PULSES) / periodBetweenTicks_us". The preprocessor will perform all constant-only math if it's all grouped together.

Done in commit a3f07010d2c54cef7d47a523ea9c4201bbf4217b. For other readers, godbolt.org with AVR GCC 13.2.0 and with flags -Os has this function going down from 0x92 bytes to 0x80 bytes: godbolt.org

Given that this is an ISR that runs once per engine pulse, these are important savings.

doppelhub / MuddersMIMA

Enable correct RPM calculation #2

Addenda