AlexT-38 / OpenGEET_ECU_firmware

Open GEET Engine Control Unit firmware
GNU General Public License v3.0
1 stars 0 forks source link

MAP sensor sampling is insufficient. #10

Closed AlexT-38 closed 1 year ago

AlexT-38 commented 1 year ago

Problem

Air pressure at the intake varies much more rapidly than the current sample rate can keep up with. Each engine rotation takes around 15-40ms, but the ADC is sampled only every 100ms. This causes aliasing of the signal, due to the Nyquist limit.

Solutions

At the very least, the signal needs to be filtered before sampling. Ideally, the sample rate needs to be increased to at least 100Hz, 10ms between samples.

Details

Maximum sampling rate

The ADC takes 104us to perform a conversion. Sampling all 16 channels at this rate would give a maximum sample rate of 600Hz, 1.6ms. However, overheads for processing the data and interrupts makes this closer to 400Hz, 2.5ms. This is impractical as the processor would not be able to do much else.

Multiple sampling rates

Only the MAP inputs need a higher sample rate. If we assume a maximum of 5 sensors (1 on the engine intake, and 1 on each reactor port), sampling takes ~0.6ms. Using an efficient interrupt would reduce the total over head per sample to perhaps around 10us per sample.

Sample triggering

Using a timer to trigger the conversion gives us control over the exact sample rate. From a data point of view it would be better to keep the samples of each channel close together in time, rather than spreading them out. So the timer would have to trigger the first sample, and the ADC interrupt changes channels and triggers the next sample.

Other ISR's

RPM

The only other time critical code is the RPM counter interrupt. Presently this only needs to be accurate to within 1ms, and if ever increased it would likely to be no more than 100us. This is 10x-100x greater than the approximate time to service the ADC interrupt, so RPM counter accuracy should not be affected. The RPM counter should be using a timer configured for Input Capture, now that we have the MEGA's additional 16bit timers. This would make ISR latency irrelevant to counter accuracy.

Serial port

There are actually two more ISRs, the serial transmit and receive handlers.

At 1MBaud, characters are transmitted in 10us. If any other ISR takes longer than this to execute, out going rate will drop, and incoming characters may be dropped. The receive buffer is two bytes plus the shift register, so it would take 30us before an overrun occurred. It's unknown how long the serial ISRs take, but if the serial Tx, RPM input and ADC interrupts all occurred at the same time, they would together have to complete in less than 29us to avoid serial buffer overruns at 1MBaud.

Decreasing the baud rate would help here, but it would also increase the time spent transmitting log data. Records are currently around 400 characters long, and would take at least 4ms to transmit. Increasing the sampling rate would also increase the amount of data to be sent, by around 260 characters per channel, so maybe 2000 characters per records, which would take 20ms to transmit. Reducing the baud rate to 500kBaud would double that to 40ms. This is that absolute most amount of time that can be spend doing anything in the main loop, and supporting this would require the serial transmit to be separately scheduled. Transmitting data in binary format would reduce this time substantially, but at the expense of human readability.

At present there is no support for reading commands from the serial port, but it would be useful as an option for setting or reading configuration values.

Synchronisation

Finally, the timer triggering sampling events would need to be synchronised with changes of record. While it should naturally synchronise if the correct timer TOP value is given, there's no guarantee.

There also needs to be time available for the 10Hz channels to be sampled. This might best be done by having the other channels added in turn on to the end of each 100Hz sampling event. That is, each time the timer triggers sampling, all the map sensors plus one other analogue input are sampled, and each time the extra channel progresses through the slower rate channels. The total number of low rate channels would be limited by the ratio of fast to slow rates. At 10 and 100Hz, a total of 10 channels could be sampled at 10Hz, while at 10 and 200Hz, 20 inputs could be sampled. As there are only 16 channels, and at most 6 possible fast sensors, 10 to 1 sampling rate ratio works nicely.

AlexT-38 commented 1 year ago

Note that the built in serial transmit handler doesn't use ISRs AFAIK.

AlexT-38 commented 1 year ago

New ADC process will look like:

Config

1) configure timer to initiate ADC conversion (assuming the correct timer is available) 2) configure timer to overflow at the desired sample rate (start with 200Hz and reduce if issues arise)

ADC task

1) read low rate channels in ADC task 2) start/reset timer and enable ADC interrupt 3) start ADC first fast rate channel and select the next channel 4) apply calibration to previously recorded values

ADC interrupt

1) record the value, accumulate values used by PID 2) if the last channel has been sampled, increment sample count 3) if the required sample count has been reached, stop the timer 4) otherwise start the next conversion and change to the next channel

Record update

1) calibrate the final set of values for this record 2) calculate averages

PID update

1) find averages from accumulated values 2) apply calibration to average 3) reset accumulators

AlexT-38 commented 1 year ago

At a sample interval of 5ms, each channel uses up to 500 bytes in the text record buffer. With up to 6 channels, this adds ~3KB to the maximum required buffer size. Maximum buffer size for a single record is currently 1500 bytes. Space is allocated for 3 buffers - a double length buffer to accommodate the header and first record, and another into which the record is rendered and then copied into the main buffer. This isn't terribly efficient and needs to be rectified.

There should be a single buffer, records should be formatted directly into that buffer, and when the buffer is filled before the end of formatting the record, that record should be dropped.

Preferably this should be done in a way that minimises code changes. Pehaps by replacing the String class with a custom class that implements the same append/concat operators, but which flags as invalid and stops appending once the underlying array is full. The length of the buffer is only updated after performing all concats, and only if still flagged as valid.

AlexT-38 commented 1 year ago

Since commit 9c5f43e in branch fast-map-sampling, every 3-4 records, the last MAP0 value and the first TRQ value are recorded as '21'.

This value of '21' is not being read by the ADC or ADC_ISR processes.

With the number of TMP sensors set to 0, the last MAP0 value and the first TRQ value are adjacent.

AlexT-38 commented 1 year ago

As a MAP value, 21 corresponds to a raw ADC reading of 1165. 1165 is the current calibration's torque sensor zero offset, that is the value in mN.m displayed when the torque sensor is disconnected, or otherwise returns zero counts. I need to check the index of the last fast ADC reading. It seems likely that the sample count is being incremented past the end of the array, such that when the data is calibrated and printed to the string buffer by finalise_record(), the first TRQ value is being calibrated (1165 maps to 21) and being printed both at the end of the MAP array and the start of the TRQ array.

AlexT-38 commented 1 year ago

Sample rate is 100+/-1 per record, with 99 and 101 samples occurring roughly every 3.5 records. 99 sample records occur almost always after a 101 sample record, but occasionally 1 record after that.

It shouldn't be recording more than 100 samples, so that needs fixing.

Fast ADC reads are synchronised with the analogue process, but record swaps are controlled by the finalise record process. Each process is timed with 1ms accuracy, and fast ADC samples are 5ms apart. Each task is executed in it's own time slice, and have been measured and found to execute entirely within their allotted time slots. The samples recorded by the last analogue task prior to a finalise record task overlap with that record task. This variation in sample rate must be due to millisecond jitter in the task execution times of two pairs of record/ADC tasks adding up to be greater than 5ms (or maybe 2.5ms?)

Two possible solutions are : 1) Improve the timing accuracy for the affected tasks 2) Increase the fast ADC sample array size by 1

I've tried (1) before, using a dedicated timer, and it didn't go well. Simply using micros() instead of millis() would probably suffice, at the expense of needing to use long ints for , or right shifting micros() to the desired precision (16us for 500ms records). This would also be relatively easy to test, so lets try that first.

AlexT-38 commented 1 year ago

Oversampling no longer occurs, but roughly every 8 records, a sample is dropped.

I can see absolutely no reason why the number of samples went above maximum. Sample count is only incremented on one line in the ADC ISR, and only if it is less than the maximum sample count. This is disturbing and should be investigated further by reducing the max sample count and checking if it stops at that value or continues.

AlexT-38 commented 1 year ago

typo: semi colon at 'if' EOL prior to protected block.

Having fixed that I've tried increasing the sample rate slightly so that there's an average of 101 samples. This requires a timer interval of 500/101 = 4.95ms, however, only 100 or 105 samples could be selected, perhaps due to the timer being restarted every ADC update? Regardless of the exact timing, there's always one record in every 7-9 that has one less sample. It might be better to synchronise the timer with only the record process. When reading the slow channels, simply suspend the timer overflow interrupt.

To be tried out tomorrow.

AlexT-38 commented 1 year ago

Done. Working, consistently 100 samples per record. Parked micros() based timing for finalise_record() and process_analog() in a side branch.