Monitor serial communication between two devices.

Hi!

First post :slight_smile: Not so new to the forum, but not active. Thanks for a great forum :slight_smile:

I am writing code without beeing able to test it because I am half way round the globe, so it may be stupid to post questions without trying it first. But I am sitting here waiting to travel home and would very much appreciate some feedback to my code.

I want to tap into a serial communication between my cars ECU and my computer OBD interface, to read out the init sequence timing (0x33 - 5 baud via K-line I believe) and the communication (10400 baud I belive).

The plan is to print to serial monitor the duration for each pulse, if it is high/low, and if it is receive or transmit to the microcontroller.
Oscilloscope would probably been a better tool, but I dont have one and want to try this :slight_smile:

I am using a library that I wrote for an other project I did for my car, so apologize if it makes the code difficult to read. I wrote the library for myself to learn from the process and hide and reuse some code, and it works good for me. I have only been using the “DigIn” class in the library and it is just some debounce, input state and edge detection, ignore the others classes if you look at the files.

Hardware: I plan to use a couple of signal diodes and pulldown resistors to each input and common ground.

This code compiles, but I guess there is some(a lot) of fixes to do.

So the question: Will it work at all like this or with some debugging?
All feedback on code improvement, suggestions etc is very much appreciated :slight_smile:

#include <MyLib0.h>

#define pinRX 2
#define pinTX 3

DigIn rx(pinRX, 0); //Digital Input class, rx instance, pin 2 - serial receive, no debounce
DigIn tx(pinTX, 0); //Digital Input class, tx instance, pin 3 - serial transmit, no debounce

uint32_t ms = 0; // milliseconds since last program re/start
bool comActive = 0; // control bit for communication active
uint32_t comSleepTime = 5000;// needs adjustment, no data for X seconds, comActive = 0

// Declare function, takes an instance of DigIn class and direction (TX or RX) as input argument.
void monitorSerial(DigIn inp, String dataDir);

void setup(){
	
	Serial.begin(9600);
	rx.begin();
	tx.begin();
}

void loop(){

	ms = millis();

	tx.read();// read tx line digital input
	rx.read(); // read rx line digital input

	monitorSerial(tx, "TX");// call function with tx instance.
	monitorSerial(rx, "RX");// call function with rx instance.
}


void monitorSerial(DigIn inp, String dataDir){

	static uint32_t bitTime = 0; // local variable to calculate time
	static uint16_t bitNr = 0; // counter variable for bitLengths and bitStates arrays
	uint32_t bitDuration[20]; // array containing duration of bit state in msec.
	bool bitState[20]; // array containing actual state of bit, 0/1

	if (inp.pEdge() || inp.nEdge()){// a positive or a negative state change detected
		
		comActive = 1;
		
		if (bitNr == 0){
			bitTime = ms;// reset timer variable
			if(dataDir == "TX")Serial.println("Transmit:");
			else if(dataDir == "RX")Serial.println("Receive:");
		}
		else{
			bitState[bitNr] = inp.state();
			bitDuration[bitNr] = ms - bitTime;
			Serial.print(dataDir);
			Serial.print(" Bit nr: ");
			Serial.print(bitNr);
			Serial.print(" State:");
			Serial.print(bitState[bitNr]);
			Serial.print(" Duration: ");
			Serial.print(bitDuration[bitNr]);
			Serial.println(" mSec");
			bitTime = ms;// reset timer variable
		}
		
		bitNr ++; // increase counter by 1
	}
	
	if (comActive){
		if (ms - bitTime > comSleepTime){
			comActive = 0;
			bitNr = 0;
		}
	}
}

MyLib0.zip (2.52 KB)

#include <MyLib0.h>

It’s a good thing most people can come up with better names than that.

A header file should define ONE class and the name of the header file should be the same as the class, with a .h extension. A source file should implement ONE class and the name of the source file should match the class, with a .cpp extension.

	tx.read();// read tx line digital input
	rx.read(); // read rx line digital input

Transmit implies sending. It doesn’t make sense to read an output pin. Better names are in order.

void monitorSerial(DigIn inp, String dataDir){

Using Strings to hold constant values is a waste of resources. There is no reason to wrap a string in a String when the string won’t change.

Your program is going to spend far more time sending data than it is reading data.

There is no substitute for actually testing the code.

PaulS:
It's a good thing most people can come up with better names than that.

A header file should define ONE class and the name of the header file should be the same as the class, with a .h extension. A source file should implement ONE class and the name of the source file should match the class, with a .cpp extension.

Totaly agree, I just never planned on publish it. But thats no excuse

Transmit implies sending. It doesn't make sense to read an output pin. Better names are in order.
Well, in this case I am actually reading data of rx and tx lines between two separate devices. But if it was the tx of the arduino I agree. But the name could always be clearer

Using Strings to hold constant values is a waste of resources. There is no reason to wrap a string in a String when the string won't change.
I will come up with a better solution on this.

Your program is going to spend far more time sending data than it is reading data.

There is no substitute for actually testing the code. Agree on that :slight_smile:

Thanks a lot for your feedback PaulS. Reply to your comments in red.