Reading Pins on ATMega328PB

I am using an ATMega328PB to read a centronics port (USB to Centronics).

I have implemented all of the signals, and use an interrupt to monitor PCINT20 for the STROBE signal going low which indicates that there is data ready to be read on the port.

The code works fine and reads each byte sent through to the centronics port except for the final byte which is overwritten by 255 (all pins high).

The interrupt code is:

ISR(PCINT2_vect) {
  prn.strobeHigh = !prn.strobeHigh;
  if (!prn.strobeHigh) {
    // Data received
    prn.recdChar = digitalRead(PRN_D0) +
          (digitalRead(PRN_D1) << 1) +
          (digitalRead(PRN_D2) << 2) +
          (digitalRead(PRN_D3) << 3) +
          (digitalRead(PRN_D4) << 4) +
          (digitalRead(PRN_D5) << 5) +
          (digitalRead(PRN_D6) << 6) +    
          (digitalRead(PRN_D7) << 7);
    // Move sequencer on to the step to set the BUSY signal and process the data
    prn.seq.step = 2; 
  }
  // handle pin change interrupt for PCINT16 to PCINT23 here
}  // end of PCINT2_vect

void setup() {
  prn.strobeHigh = true;

  // based on https://forum.arduino.cc/t/interrupt-on-pin-4-pcint20/651245
  // pin change interrupt to note state change of the PRN_STROBE pin
  PCMSK2 |= bit(PCINT20);  // Mask PCInt20 on the Interrupt (bit 4) PCMSK2 covers PCINT16 to PCINT23
  PCIFR |= bit(PCIF2);     // Clear any outstanding interrupts (PCIF2 is any change interrupt on PCINT16 to PCINT23)
  PCICR |= bit(PCIE2);     // Enable pin change interrupts Control Register for PCINT16 to PCINT23
}

all this can be replaced by

prn.recdChar = PORTD;

Unfortunately, I don't think that would work - we have more than 8 input pins and the lines are not all on one port. As I say we are reading the data correctly except for the very last byte for some reason!

Lots of potential reasons, based on the information you failed to provide, such as speed of transmission, the way it is wired, the actual code (you said "lines are not all on one port" which means this is not your actual complete code), the amount of data sent, how that data is stored in your code for later processing, etc.

Nonetheless your posted code seems very inefficient and can be improved on.

digitalRead() calls can be replaced by direct PORT calls. You can read and set individual bits using & and ^ operators. Those things make the ISR much faster, which may be critical for your application to also get that last byte in time.

Also, are you sure you want to toggle prn.strobeHigh before checking whether you have data? This sounds a bit odd. But then again you didn't provide any information on the signal you're trying to read so I'm probably just wasting my time looking at it so critically.

The timing is based on the Centronics protocol which can be seen at:
http://www.theiling.de/parport/centronics.html

The data pins (we actually use A0 to A7) can vary all the time until the strobe signal on pin 4 goes low.

We then read the data at that point, and once that is read, set the BUSY signal until we have processed the data, making the ACK signal low at that point.

I have even tried settign the BUSY signal in the ISR before reading the data, but that does not prevent the 8 A0-A7 pins going high on the last byte.

It is peculiar that only the last byte is affected no matter how long a string I send to the centronics connector - as we have pullups on all of the input pins, this is obviously causing the pins high

More than 8 pins to read the 8 bits?

Why?
To speed up and simplify code, select lines on one port.

Buy the way, your A0-A7 pins actually are in the same MCU port - PORTC

I am agree with @wvmarle - the code above looks wrong.
How can you trigger of data reading by strobe signal if you yourself invert the strobe every time you enter an interrupt? As a result, at most at the second interrupt firing you will have a low strobe and you will read incorrect data.

I don't invert the strobe - that strobeHigh is just a flag to keep track of the current signal status (high or low)

According to the protocol, the data should only be read when the strobe changes to low.

Following advice above, I have changed the ISR to:

ISR(PCINT2_vect) {
  prn.strobeHigh = !prn.strobeHigh;
  if (!prn.strobeHigh) {
    // Apply busy signal
    digitalWrite(PRN_BUSY, true);
    prn.recdChar = ((PINE & B00001100 ) << 4) | (PINC & B00111111); 
    prn.seq.step = 2;
  }
  // handle pin change interrupt for PCINT16 to PCINT23 here
}  // end of PCINT2_vect

However, this does not prevent recdChar being 255 when the final character is received.

Just a thought - am I handling the creation of the interrupt correctly? The interrupt should be triggered ONLY when the strobe pin goes high or low (ie on a change signal)

Then you can just check the status of that input pin. No need to set a variable.

ISR(PCINT2_vect) {
  if (digitalRead(flagPin) == HIGH) {
  ...
  }
}

Anyway, you still didn't post the rest of the code. The actual code, the code that you're running now, not a snippet of something else (like apparently in your first post).

Usually the part you didn't post is where the real error is hiding out.

Interesting reading. Especially these bits:

Makes me wonder what device exactly you're trying to communicate with, and whether you're sure the device behaves the way you think it should.

I didn't want to share the full code, but the full project can be downloaded from dropbox

I do wonder if the issue is that the interrupt only acts on the change of the strobe signal, rather than the falling edge - as the strobe signal is only held low for 5us

At the moment, we are just using a USB to centronics (DB25) adaptor from Windows. I have an existing Retro-Printer device (running on a Raspberry Pi) which does the same job and that works with the adaptor so I am fairly confident the cable is acting as it should.

What you should do:

Create a minimal sketch, that still shows the problem. Remove anything else. That's a very good problem solving strategy, as it very often helps to find and solve the problem.

If it still happens, post that here in the forum. Not external links, I've better things to do.

Minimal sketch:

/********************************************************************************
 * PrinterCaptureInterrupt.ino
 * ------------------
 * Monitor a parallel port printer output and capture each character. Output the 
 * character on the USB serial port so it can be captured in a terminal program.
 *
 * By............: Rich Mellor - simple based on version by Paul Jewell 29th January 2015
 * Date..........: 5th December 2023
 * Version.......: 0.1a
 * Modification : Change Interrupt Routine so Arduino respond "faster" to Printer
 *                Writing Busy Signal directly from interrupt routine
 *                Ecirbaf 12 Jan 2017
 *                Test on a PC with a "generic Printer Text only" printer
 *                Printing a test page OK
 *                Even if using somme accent characters
 *                Depend How your Terminal Software is handling that.
 *-------------------------------------------------------------------------------
 * Wiring Layout
 * -------------
 * 
 * Parallel Port Output               Arduino Input
 * --------------------               -------------
 * Name      Dir.   Pin                Name    Pin
 * ----      ----   ---                ----    ---
 * nSTROBE    >       1................INT0      2 (as interupt)
 * DATA BYTE  >     2-9.......................3-10    
 * nACK       <      10.........................11
 * BUSY       <      11.........................12
 * OutofPaper <      12................GND
 * Selected   <      13.................5v
 * GND        <>  18-25................GND
 *-------------------------------------------------------------------------------
 ********************************************************************************/


#include "Arduino.h"
#include "pins_arduino.h"


//define the pins
//25 pin printer port
#define PRN_SelectIn 2  // DB25 Pin 13 SelectIn OUTPUT
#define PRN_PERROR 3    // DB25 Pin 12 Paper Out OUTPUT
#define PRN_STROBE 4    // DB25 Pin 1 Strobe INPUT
#define PRN_AutoFeed 5  // DB25 Pin 14 Autofeed INPUT
#define PRN_NFault 6    // DB25 Pin 15 Error OUTPUT
#define PRN_INT 7       // DB25 Pin 16 Reset INPUT
#define PRN_SELECT 8    // DB25 Pin 17 Select INPUT
#define PRN_ACK 9       // DB25 Pin 10 Ack OUTPUT
#define PRN_BUSY 10     // DB25 Pin 11 BUSY OUTPUT

#define PRN_D0 A0  // DB25 Pin 2 DATA_0 INPUT
#define PRN_D1 A1  // DB25 Pin 3 DATA_1 INPUT
#define PRN_D2 A2  // DB25 Pin 4 DATA_2 INPUT
#define PRN_D3 A3  // DB25 Pin 5 DATA_3 INPUT
#define PRN_D4 A4  // DB25 Pin 6 DATA_4 INPUT
#define PRN_D5 A5  // DB25 Pin 7 DATA_5 INPUT
#define PRN_D6 A6  // DB25 Pin 8 DATA_6 INPUT
#define PRN_D7 A7  // DB25 Pin 9 DATA_7 INPUT

byte myChar;
bool strobeState = true;

enum States {
  READY,
  BUSY,
  ACK
} State;

ISR(PCINT0_vect) {
  // state = !state;
  // handle pin change interrupt for D0 to D7 here
}  // end of PCINT0_vect

ISR(PCINT1_vect) {
  // state = !state;
  // handle pin change interrupt for A0 to A5 here
}  // end of PCINT1_vect

ISR(PCINT2_vect) {
  if (digitalRead(PRN_STROBE) == HIGH) {
    // Set the Busy Signal true - to stop the centronics sending more data
    digitalWrite(PRN_BUSY, true);
    digitalWrite(LED_BUILTIN, LOW);      

    // Read all of the data pins quickly
    ProcessChar();
    State = ACK;
  }
  // handle pin change interrupt for PCINT16 to PCINT23 here
}  // end of PCINT2_vect

void setup() {
  // Configure pins
  pinMode(PRN_STROBE, INPUT_PULLUP);
  pinMode(PRN_D0, INPUT_PULLUP);
  pinMode(PRN_D1, INPUT_PULLUP);
  pinMode(PRN_D2, INPUT_PULLUP);
  pinMode(PRN_D3, INPUT_PULLUP);
  pinMode(PRN_D4, INPUT_PULLUP);
  pinMode(PRN_D5, INPUT_PULLUP);
  pinMode(PRN_D6, INPUT_PULLUP);
  pinMode(PRN_D7, INPUT_PULLUP);

  pinMode(PRN_AutoFeed, INPUT_PULLUP);
  pinMode(PRN_SelectIn, INPUT_PULLUP);
  pinMode(PRN_INT, INPUT_PULLUP);

  //setup the outputs
  pinMode(PRN_SELECT, OUTPUT);
  pinMode(PRN_PERROR, OUTPUT);
  pinMode(PRN_BUSY, OUTPUT);  // High indicates Busy
  pinMode(PRN_ACK, OUTPUT);   // LOW indicates an acknowledge
  pinMode(PRN_NFault, OUTPUT);

  strobeState = true;

  Serial.begin(300); 
  while (!Serial) {
    ;
  }

  pinMode(LED_BUILTIN, OUTPUT);
  digitalWrite(LED_BUILTIN, LOW);

  // based on https://forum.arduino.cc/t/interrupt-on-pin-4-pcint20/651245
  // pin change interrupt to note state change of the PRN_STROBE pin
  PCMSK2 |= bit(PCINT20);  // Mask PCInt20 on the Interrupt (bit 4) PCMSK2 covers PCINT16 to PCINT23
  PCIFR |= bit(PCIF2);     // Clear any outstanding interrupts (PCIF2 is any change interrupt on PCINT16 to PCINT23)
  PCICR |= bit(PCIE2);     // Enable pin change interrupts Control Register for PCINT16 to PCINT23



  State = READY;
  delay(100);
  Serial.println("Initialised");
}

void loop() {
  switch (State) {
    case READY:
      digitalWrite(PRN_BUSY, LOW);
      digitalWrite(PRN_ACK, HIGH);
      digitalWrite(LED_BUILTIN, HIGH);
      break;

      //    case BUSY: // nStrobe signal received by interrupt handler
      //      digitalWrite(Busy, HIGH);
      //      digitalWrite(led, LOW);
      //      ProcessChar();
      //      State = ACK;
      //      break;
      // ** All this case is made during Interrupt (Avoid some missed characters with "fast" printer) **

    case ACK:
      digitalWrite(PRN_ACK, LOW);
      delay(1);  //milliseconds. Specification minimum = 5 us    ** Reduced to 1 is ok **
      State = READY;
      break;
  }
}

void ProcessChar() {
  //byte Char = ((PINE & B00001100 ) << 4) | (PINC & B00111111); 

byte Char;
  Char = digitalRead(PRN_D0) +
         (digitalRead(PRN_D1) << 1) +
         (digitalRead(PRN_D2) << 2) +
         (digitalRead(PRN_D3) << 3) +
         (digitalRead(PRN_D4) << 4) +
         (digitalRead(PRN_D5) << 5) +
         (digitalRead(PRN_D6) << 6) +
         (digitalRead(PRN_D7) << 7);

  Serial.println(Char);
}

I am sending a string of bytes 130 to 132
Output appears as:
130
130
131
131
255
255

The main issue is that I need to test with

if (digitalRead(PRN_STROBE) == HIGH) {

It should be

if (digitalRead(PRN_STROBE) == LOW) {

However, if I use that, ProcessChar is never triggered! I guess the ISR is being triggered on the ACK signal changing from LOW to HIGH and HIGH to LOW as expected, but by the time it is triggered, it is simply too late to read the pin.

ProcessChar() should not be in an interrupt routine. It calls Serial.print(), which requires interrupts to run on most processors.

OK - I was just following the original example someone had posted some years ago:

Updated code (now at least only outputs the character once, but still not triggered if I look for PRN_STROBE == LOW) and last character still 255:

/********************************************************************************
 * PrinterCaptureInterrupt.ino
 * ------------------
 * Monitor a parallel port printer output and capture each character. Output the 
 * character on the USB serial port so it can be captured in a terminal program.
 *
 * By............: Rich Mellor - simple based on version by Paul Jewell 29th January 2015
 * Date..........: 5th December 2023
 * Version.......: 0.1a
 * Modification : Change Interrupt Routine so Arduino respond "faster" to Printer
 *                Writing Busy Signal directly from interrupt routine
 *                Ecirbaf 12 Jan 2017
 *                Test on a PC with a "generic Printer Text only" printer
 *                Printing a test page OK
 *                Even if using somme accent characters
 *                Depend How your Terminal Software is handling that.
 *-------------------------------------------------------------------------------
 * Wiring Layout
 * -------------
 * 
 * Parallel Port Output               Arduino Input
 * --------------------               -------------
 * Name      Dir.   Pin                Name    Pin
 * ----      ----   ---                ----    ---
 * nSTROBE    >       1................INT0      2 (as interupt)
 * DATA BYTE  >     2-9.......................3-10    
 * nACK       <      10.........................11
 * BUSY       <      11.........................12
 * OutofPaper <      12................GND
 * Selected   <      13.................5v
 * GND        <>  18-25................GND
 *-------------------------------------------------------------------------------
 ********************************************************************************/


#include "Arduino.h"
#include "pins_arduino.h"


//define the pins
//25 pin printer port
#define PRN_SelectIn 2  // DB25 Pin 13 SelectIn OUTPUT
#define PRN_PERROR 3    // DB25 Pin 12 Paper Out OUTPUT
#define PRN_STROBE 4    // DB25 Pin 1 Strobe INPUT
#define PRN_AutoFeed 5  // DB25 Pin 14 Autofeed INPUT
#define PRN_NFault 6    // DB25 Pin 15 Error OUTPUT
#define PRN_INT 7       // DB25 Pin 16 Reset INPUT
#define PRN_SELECT 8    // DB25 Pin 17 Select INPUT
#define PRN_ACK 9       // DB25 Pin 10 Ack OUTPUT
#define PRN_BUSY 10     // DB25 Pin 11 BUSY OUTPUT

#define PRN_D0 A0  // DB25 Pin 2 DATA_0 INPUT
#define PRN_D1 A1  // DB25 Pin 3 DATA_1 INPUT
#define PRN_D2 A2  // DB25 Pin 4 DATA_2 INPUT
#define PRN_D3 A3  // DB25 Pin 5 DATA_3 INPUT
#define PRN_D4 A4  // DB25 Pin 6 DATA_4 INPUT
#define PRN_D5 A5  // DB25 Pin 7 DATA_5 INPUT
#define PRN_D6 A6  // DB25 Pin 8 DATA_6 INPUT
#define PRN_D7 A7  // DB25 Pin 9 DATA_7 INPUT

byte myChar;
bool strobeState = true;

enum States {
  READY,
  BUSY,
  ACK
} State;

ISR(PCINT0_vect) {
  // state = !state;
  // handle pin change interrupt for D0 to D7 here
}  // end of PCINT0_vect

ISR(PCINT1_vect) {
  // state = !state;
  // handle pin change interrupt for A0 to A5 here
}  // end of PCINT1_vect

ISR(PCINT2_vect) {
  if (digitalRead(PRN_STROBE) == HIGH) {
    // Read all of the data pins quickly
    myChar = ((PINE & B00001100 ) << 4) | (PINC & B00111111); 

    // Set the Busy Signal true - to stop the centronics sending more data
    digitalWrite(PRN_BUSY, true);
    digitalWrite(LED_BUILTIN, LOW);

    State = BUSY;
  }
  // handle pin change interrupt for PCINT16 to PCINT23 here
}  // end of PCINT2_vect

void setup() {
  // Configure pins
  pinMode(PRN_STROBE, INPUT_PULLUP);
  pinMode(PRN_D0, INPUT_PULLUP);
  pinMode(PRN_D1, INPUT_PULLUP);
  pinMode(PRN_D2, INPUT_PULLUP);
  pinMode(PRN_D3, INPUT_PULLUP);
  pinMode(PRN_D4, INPUT_PULLUP);
  pinMode(PRN_D5, INPUT_PULLUP);
  pinMode(PRN_D6, INPUT_PULLUP);
  pinMode(PRN_D7, INPUT_PULLUP);

  pinMode(PRN_AutoFeed, INPUT_PULLUP);
  pinMode(PRN_SelectIn, INPUT_PULLUP);
  pinMode(PRN_INT, INPUT_PULLUP);

  //setup the outputs
  pinMode(PRN_SELECT, OUTPUT);
  pinMode(PRN_PERROR, OUTPUT);
  pinMode(PRN_BUSY, OUTPUT);  // High indicates Busy
  pinMode(PRN_ACK, OUTPUT);   // LOW indicates an acknowledge
  pinMode(PRN_NFault, OUTPUT);

  strobeState = true;

  Serial.begin(300); 
  while (!Serial) {
    ;
  }

  pinMode(LED_BUILTIN, OUTPUT);
  digitalWrite(LED_BUILTIN, LOW);

  // based on https://forum.arduino.cc/t/interrupt-on-pin-4-pcint20/651245
  // pin change interrupt to note state change of the PRN_STROBE pin
  PCMSK2 |= bit(PCINT20);  // Mask PCInt20 on the Interrupt (bit 4) PCMSK2 covers PCINT16 to PCINT23
  PCIFR |= bit(PCIF2);     // Clear any outstanding interrupts (PCIF2 is any change interrupt on PCINT16 to PCINT23)
  PCICR |= bit(PCIE2);     // Enable pin change interrupts Control Register for PCINT16 to PCINT23



  State = READY;
  delay(100);
  Serial.println("Initialised");
}

void loop() {
  switch (State) {
    case READY:
      digitalWrite(PRN_BUSY, LOW);
      digitalWrite(PRN_ACK, HIGH);
      digitalWrite(LED_BUILTIN, HIGH);
      break;

    case BUSY: // nStrobe signal received by interrupt handler      
      ProcessChar(myChar);
      State = ACK;
      break;

    case ACK:
      digitalWrite(PRN_ACK, LOW);
      delay(1);  //milliseconds. Specification minimum = 5 us    ** Reduced to 1 is ok **
      State = READY;
      break;
  }
}

void ProcessChar(byte Char) {
  Serial.println(String((int) Char));
}
   State = BUSY;

Variables shared with interrupt routines must be declared volatile, or state changes may be ignored.

Yes, good catch.

Applies to the variable myChar as well.

It seems that the actual value is read and stored correctly (though with myChar not volatile you can not be entirely sure), which begs the question: did you verify that the signal level at the pins is the signal you expect? One possible explanation of this behaviour is that your data source is misbehaving on the last byte.

I think the problem is simply timing - changes to make the variables volatile does not help, and I am still getting 255 as the last byte every time. If I send the same output to my old retro-printer using a Raspberry Pi, there is no problem - the bytes all appear as sent.

The standard says that the STROBE signal goes LOW when the data is received, but by the time the interrupt is fired, the test on the PRN_STROBE pin is always HIGH (the strobe only goes low for 5us) so I guess this is the reason for the data reading incorrectly (the cable thinks everything has been sent).

I think the only answer is to tie the BUSY to the STROBE signal in hardware....