Version 2 eChook Arduino Code discussion

...

I took a look at a time based, rather than poll based, calculation for the wheel speed and motor RPM this morning. It turned into quite a big code reorganisation as it really hit me that over time, the changes to that original code have added up and there is a fair bit of redundant code and incorrect comments remaining - sorry about that!! In the years since we originally wrote that code I think my coding style has changed significantly too.

So, I have made a new branch of code on github and split the file up into smaller chunks to hopefully make everything easier to find, and more obvious where users can add their own code to the eCHook. It also includes a few improvements that have been found/suggested over time.

I have also done a first attempt at the new motor and wheel speed code, and added a toggle to enable it under an 'Experimental' heading in calibration.h. It's currently enabled by default. I'm getting a little ahead of myself sharing it, as I've hardly tested it yet, but hopefully you can see what I'm trying to do. Feel free to give it a shot, just don't be surprised if it doesn't work right now

New dev branch of the code is here: github.com/eChook/eChook-Arduino-Nano/tree/dev 

Any feedback welcome as always

You have been busy Rowan, great work. Can I make a small additional request for an update to the dev branch (yeah, I'm too lazy to make a pull request ... ) - how about changing the speed and current smoothing arrays from int to float to avoid discretizing the calculated averages into thirds and quarters respectively.
Thanks!
Keith

Rowan - Rowan, no other optimisations as such to add. A couple of things I have done though are:
(a) added additional code in sendData (both overloads) so that when in DEBUG_MODE the identifier and value are printed in plain text rather than being encoded. I've found this useful when debugging eChook code as the output is easily viewed in the serial monitor window in the IDE using a wired connection to the nano in place of bluetooth. Avoids needing to run an additional app to decipher the code (though (as you are aware) I have also put together a windows app which does just that!!)
(b) added macros to Calibration.h to allow different configuration parameters for different cars whilst sharing the same code files. I found it easier to remember to change a #define in Calibration.h than to manage multiple calibration files for multiple cars, or managing multiple copies of the code. Here's an abridged version of our current Calibration.h (I've also added a couple of additional calibration values CAL_INVERT_BRAKE and CAL_THROTTLE):


/*
* This file contains all the values used to calibrate your eChook board.
* By seperating these it makes it possible to update to newer code without
* having to copy your calibrations across - just make sure you keep this file!
*
* The values provided by default are for the eChook's team dev board, but each
* board is different. These defaults will give readings in the right ballpark,
* but for accuracy, see the documentation to calibrate your own board.
*
* Every variable in this file begins with CAL_ so that it is identifiable in  
* the main code as being defined in this file.
*
*/

//================================================================================================================================
//
// SELECT eChook BOARD -- IMPORTANT !!
//
// Only ONE of the following BOARD_xxxxxx should be defined at any one time. Other defines should be commented out.
//
   //#define BOARD_ELECTRON
   #define BOARD_PHOTON
//
//================================================================================================================================

// Banchory Academy Electron (nee Fifty Shades of Green)

#if defined(BOARD_ELECTRON)
                                                                           // Bluetooth Settings
   const String CAL_BT_NAME     = "eChook";                                // Whatever you want to name your car's bluetooth
   const String CAL_BT_PASSWORD = "1234";                                  // Changing password from default "1234" tends not to work yet - apologies!
   
                                                                           // Car Specific Settings
   const int   CAL_WHEEL_MAGNETS        = 3;                               // Number of magnets on wheel
   const int   CAL_MOTOR_MAGNETS        = 6;                               // Number of magnets on motor shaft for hall effect sensor
   const float CAL_WHEEL_CIRCUMFERENCE  = 1.429;                           // Outer circumference of tyre, in Meters. i.e. the distance travelled in one revolution
   const bool  CAL_INVERT_BRAKE         = false;                           // Invert brake output value - needed if brake input is normally low
   
                                                                           // Board Specific Calibrations
   const float CAL_REFERENCE_VOLTAGE   = 5;                                // Voltage seen on the arduino 5V rail
   const float CAL_BATTERY_TOTAL = (82000.0 + 16000.0) / 16000.0 * 0.993;  // which should be 6.125, not 6.15 as provided originally. Final multiplier added to make eChook battery total match Fluke meter reading
   const float CAL_BATTERY_LOWER = (82000.0 + 16000.0) / 16000.0 * 1.003;  // which should be 6.125, not 6.15 as provided originally. Final multiplier added to make eChook battery lower match Fluke meter reading
   const float CAL_CURRENT             = 37.55;                            // Current Multiplier - See documentation for calibration method
   
                                                                           // Board and Sensor Specific Calibrations
   const float CAL_THERM_A = 0.001871300068;                               // Steinhart-Hart constants - See documentation for calibration method
   const float CAL_THERM_B = 0.00009436080271;
   const float CAL_THERM_C = 0.0000007954800125;

                                                                           // Throttle adjustment
   const float CAL_THROTTLE = 100.0 / 96.5;                                // Throttle scalar to account for throttle pot being supplied +5 from power controller and not from eChook
   
#endif

// Banchory Academy Photon
#if defined(BOARD_PHOTON)
                                                                           // Bluetooth Settings
   const String CAL_BT_NAME     = "___kChook___";                          // Whatever you want to name your car's bluetooth
   const String CAL_BT_PASSWORD = "1234";                                  // Changing password from default "1234" tends not to work yet - apologies!
   
                                                                           // Car Specific Settings
   const int   CAL_WHEEL_MAGNETS        = 6;                               // Number of magnets on wheel
   const int   CAL_MOTOR_MAGNETS        = 3;                               // Number of magnets on motor shaft for hall effect sensor
   const float CAL_WHEEL_CIRCUMFERENCE  = 1.181;                           // Outer circumference of tyre, in Meters. i.e. the distance travelled in one revolution
   const bool  CAL_INVERT_BRAKE         = true;                            // Invert brake output value - needed if brake input is normally low

   const float CAL_REFERENCE_VOLTAGE   = 5;                                // Voltage seen on the arduino 5V rail
   const float CAL_BATTERY_TOTAL = (82000.0 + 16000.0) / 16000.0 * 1.000;  // which should be 6.125, not 6.15 as provided originally. Final multiplier added to make eChook battery total match Fluke meter reading
   const float CAL_BATTERY_LOWER = (82000.0 + 16000.0) / 16000.0 * 1.000;  // which should be 6.125, not 6.15 as provided originally. Final multiplier added to make eChook battery lower match Fluke meter reading
   const float CAL_CURRENT             = 37.55;                            // Current Multiplier - See documentation for calibration method

                                                                           // Board and Sensor Specific Calibrations
   const float CAL_THERM_A = 0.001871300068;                               // Steinhart-Hart constants - See documentation for calibration method
   const float CAL_THERM_B = 0.00009436080271;
   const float CAL_THERM_C = 0.0000007954800125;
   
                                                                           // Throttle adjustment
   const float CAL_THROTTLE = 100.0 / 96.5;                                // Throttle scalar to account for throttle pot being supplied +5 from power controller and not from eChook

#endif


Ben ... I'll post a separate reply re your current sensor testing as we're probably breaking all forum etiquette by discussing multiple topics in a single reply let alone within a single thread!

-- Keith. 

Rowan ... You asked for feedback, so here we go  . Let me preface what I'm going to spout forth below by saying that this is very much my coding style and it is personal. Not saying its right, not saying other styles are wrong, we all evolve our styles over time. I've worked on software development projects where there have been defined coding standards, some were good, some not so good, but I think every programmer on the development  teams managed to fault the coding standards where they didn't conform with their own style. So please (please) just take the following as reflecting my style and preferences, and of course, feel free to disregard entirely. 

One of my own preferences is to keep variable declarations as local as possible to their use. I'm not against global variables being used where they need to be referenced in multiple functions. But if a variable is only used in a single function then I would declare that variable in the function. But isn't this true in the eChook code I hear you cry? Yes for most variables, but as a specific example consider the global variable:

unsigned long   lastShortDataSendTime       = 0;
This is intialised in 'setup' (master) / in 'eChookSetup' (dev) and is then only used within 'loop' (master) / in 'eChookRoutinesUpdate' (dev). My preference would be to define 'lastShortDataSendTime' in 'loop' (master) / in 'eChookRoutinesUpdate' (dev) as follows:


static unsigned long lastShortDataSendTime = millis();
The 'static' ensures the variable's value is preserved between calls, and the initialisation only happens on the first time through.

This makes the declaration much closer to its use and only within the scope of the function using it.

The same approach could be taken for a number of variables currently declared globally, including (but not limited to):
    lastWheelSpeedPollTime  - only used in 'readWheelSpeed'
    lastMotorSpeedPollTime  - only used in 'readMotorRPM'
    currentSmoothingArray - only used in 'readCurrent'
    speedSmoothingArray - only used in 'readWheelSpeed'

How much do I care about this .... in reality ... not enough to have made me change this in the version of the eChook code that we use here  .

Just a thought .................. Keith