[Eric H]

Hey there.
On the race car I've been moving the new dashboard to Serial3 so when we go to the dyno in a couple of weeks the laptop can plug in to the primary serial to tune and the dash will still work.
It doesn't. When Serial3 is running at 10Hz Tunerstudio drops to a correct update every couple of seconds with errors generated sometimes. Unplug the dashboard and in a couple of seconds TunerStudio picks up and works correctly.

I started looking at the code. It's a shambles.
There's comms.cpp with code to handle main serial and serial3. The main serial code has an asynchronous send that's broken if you were to use main serial and serial3 at the same time, however the comms.cpp code handling serial3 is all dead and never used. The main speeduino.ino code needs to know details of the implementation to handle the async sends, this is a bad design decision.

There's cancomms.ino for handling true CANBUS and serial3. The merging of the two communication methods is a flawed idea. The data retrieved from serial3 is different and incomplete compared to main serial. There is no valid reason for serial3 to be handled differently than main serial. CANBUS is a different matter altogether and deserves a separate implementation that serves the protocol.

Then there's newcomms.cpp, an in-progress clean up that adds the CRC checking to the protocol. I haven't delved too deeply yet.

I propose that I clean up comms.cpp to handle both main serial and serial3 with proper async transmission.
I will also remove the non-CANBUS parts from cancomms.cpp to allow a clean, non-compromised implementation of the CANBUS protocol. I would like to, but won't without the go-ahead make the live data retrieved by CANBUS to be the same as the data from the UART serial ports. It's silly that the live data is different for the same commands. If you want different data, use a different command.

I haven't found the bug yet that screws up main serial when using serial3 yet, but I stopped looking when the code is need of deeper work.

Any comments? I want to bang this out over the next couple of days so I can get the car to the dyno.

-----EDIT------
I started cleaning up the code and came to the realization that comms.cpp isn't used at all.
This is not a good thing. Remove dead code. If you need to go back and look at it, that's what source control is for.
newComms is still not great code in places. You shouldn't dump all the code in the parser switch statement, it makes the code very hard to inspect. It does ugly things with globals. Code with global side-effects are very hard to debug when things go wrong.

I'll keep looking and post more as I dig.

----- More Edits ------
comms.cpp is used, but it is called from newComms for legacy stuff. So there's four implementations for two things.
The comms data refactor should use the same method of buffering and sending data as newcomms.
Simplify everything.

[pazi88]

Hey there.
On the race car I've been moving the new dashboard to Serial3 so when we go to the dyno in a couple of weeks the laptop can plug in to the primary serial to tune and the dash will still work.
It doesn't. When Serial3 is running at 10Hz Tunerstudio drops to a correct update every couple of seconds with errors generated sometimes. Unplug the dashboard and in a couple of seconds TunerStudio picks up and works correctly.

I'm 95% that this is just faulty implementation of the Serial3 reading. I have had same problem and the reason was in my code. Not in speeduino code. Of course speeduino implementation isn't perfect and bug free but still, I would look into your own implementation of reading serial3 first.

Also you can't just modify what the serial3 sends out freely. There is dozens of implementations using it currently as it is, so if you change what and how serial3 sends out, you will break those 3rd party implementations using serial3. The purpose of serial3 is different than the main serial. The main serial is for TS and tuning the ecu. Serial3 (CANserial) is to read live data from the ecu. Not tune it, so it works differently.

And comms.cpp is used if deactivate new comms protocol from TS. The move to new comms has just happened just few weeks ago, so old stuff is kept there until new comms is in 100% working state.

[Eric H]

It's not faulty code on my side in this case, my dashboard is working perfectly.
It's TunerStudio that doesn't work properly over USB while serial3 is active.

I understand that I shouldn't just change things unilaterally, that's why I started this discussion.
That, and it's currently broken. If I'm going in to fix things, I might as well fix it right.

Right now, CANSerial is different than serial3 and that's fine.
What is not fine is serial3 different than main serial. If you wanted to lock out tuning from serial3 that's easily doable without duplicating code or trying to shoehorn CAN and plain serial into the same messy implementation. CAN is a different protocol and it may or may not have the same data payloads available.

[pazi88]

It's not faulty code on my side in this case, my dashboard is working perfectly.
It's TunerStudio that doesn't work properly over USB while serial3 is active.

Yes, I had same issue. The data reading from serial3 was fine, but problems in TS connection. The problem was because my code was handling serial3 wrong and causing hickups in Speeduino that messed the TS connection. But can't anymore remember what was the reason for that.

[Eric H]

It's broken code in Speedy if using one serial port affects another. They don't even share a codepath right now.

[pazi88]

It's broken code in Speedy if using one serial port affects another. They don't even share a codepath right now.

Ok, now I remember. The serial 3 reading needs to be request - response -request -response etc. order. If your code IE tries to request data too fast (before reading the previous response fully), you will quickly run into this If in speeduino code: if ( ((mainLoopCount & 31) == 1) or (CANSerial.available() > SERIAL_BUFFER_THRESHOLD) )

So if there is too many requests in the buffer, the code reads those almost every loop, instead of checking the serial3 based on main loop counts. And that can cause problems in other places, because looping the main loop takes too much time.

I don't know if that is happening in your case, but it's one possibility.

[Raku]

Hasn't had any issues using serial3 so far. Both dash and laptop connects via Bluetooth and both works without any disconnects.

[Eric H]

Ok, now I remember. The serial 3 reading needs to be request - response -request -response etc. order. If your code IE tries to request data too fast (before reading the previous response fully),

I wait for the previous request to finish and send a new request no sooner than 100ms from the previous request.

[Eric H]

Hasn't had any issues using serial3 so far. Both dash and laptop connects via Bluetooth and both works without any disconnects.

Have you upgraded to 202202?

[Eric H]

I spent a lot of time last night reorganizing newComms. Saved some code and 100+ bytes of ram without changing any functionality.
Starting to move on to the next phase where I genericize the serial ports so the code can work with any serial port I cam across this dandy:

Code: Select all

if (serialReceivePending == false)
  { 
    serialBytesReceived = 0; //Reset the number of bytes received as we're starting a new command

    //New command received
    //Need at least 2 bytes to read the length of the command
    serialReceivePending = true; //Flag the serial receive as being in progress
    byte lowByte = Serial.read();

    //Check if the command is legacy using the call/response mechanism
    if((lowByte >= 'A') && (lowByte <= 'z') )
    {
      //Handle legacy cases here
      serialReceivePending = false; //Make sure new serial handling does not interfere with legacy handling
      legacySerial = true;
      currentCommand = lowByte;
      command();
    }
    else
    {
      while(Serial.available() == 0) { } //Wait for the 2nd byte to be received (This will almost never happen)

      byte highByte = Serial.read();
      serialPayloadLength = word(lowByte, highByte);
      serialBytesReceived = 2;
      cmdPending = false; // Make sure legacy handling does not interfere with new serial handling
      serialReceiveStartTime = millis();
    }
  }

This code is guaranteed to fail if a new command has between 65 and 122 inclusive in the LSB of the length field.
The only way to do this kind of thing is to put a magic value at the start of the new commands that is guaranteed to be unique.
The standard way is to make a message header something like this:

Code: Select all

struct MessageHeader
{
  uint32_t magic;  // must be "SPDY" or some other extremely unlikely four byte value to show up in the data
  uint16_t command;
  uint16_t size;
  uint32_t dataCRC; // does not include header in CRC calcs
};

I Haven't seen any docs on the new command format. I'd like to get some discussion in on this issue.