7-bit support seems to be missing on Teensy 4

It turns out that without the fix, the transmit properly sends 7-bit data, but receive still sometimes sets the 8th bit high. I need to explore more.
 
I haven't checked any details (even checked the manual), but could it be related to parity checking?
Even in no-parity modes, there could be some register flag that causes the 8th bit to be used for something similar, like framing error or such, that needs to be configured/disabled differently for 7-bit serial.

(I'm pretty sure you are well aware of this and I'm providing nothing useful here, but I had to post just in case you hadn't thought of it yet.)
 
Thank you for adding that. I think it's important information to think about if one doesn't know much about the serial ports.
 
On my journey to understanding this better — It's been a while since I've explored this in depth (for TeensyDMX) — I think I'm re-discovering that the "data" includes everything but the start and stop bits. This means that my proposed solution, setting M7, will really cause a 6-bit data byte if there's parity. If I'm right, then my solution isn't correct. I'm closer...
 
@Nominal Animal I think you're right about that top bit being the parity bit. Since the serial bit stream comes in with the least significant bit first, that top bit is the last bit, which is the 8th bit in a 7-bit parity-checked format. My received data confirms this. Now onto a fix...
 
Okay, I think I got it. There has to be an appropriate mask depending on the mode. I updated the PR.
 
Note: In all cases like this, it would help if showed a small sketch showing what you are trying to do:

For example, what are you calling: SerialX.begin(...) with?

That is, the 7-bit support that is in Teensyduino, currently always sends and receives 8 bits.
Code:
#define SERIAL_7E1 0x02
#define SERIAL_7O1 0x03
Following the standard of the 8th bit is the parity bit. Which is either Evan or Odd.
We have the same Symbolic define as Arduino shows in https://www.arduino.cc/reference/en/language/functions/communication/serial/begin/

They do have other defines that we do not have nor have we implemented.
For example they do have do have defined, things like:
Code:
SERIAL_5N1
SERIAL_6N1
SERIAL_7N1
Which probably the last one is what you are looking for.

I did not look completely through your PR, but what I would expect the code to do would be, for example in the T4.x code base:
Define: Serial_7N1 and figure out a bit for it.

Do not change the functionality of SERIAL_7E1 or SERIAL_7O1 as they are doing what they should be doing. That is sending 8 bits with parity in upper bit.

Instead, the code needs to change the configuration to be in 7 bit mode.
On the Teensy 4.x you would change the LPUART Control Register (CTRL) section 49.4.1.8 Page 2865
Look at the bit labeled: M7
This bit is also defined in imxrt.h : #define LPUART_CTRL_M7 ((uint32_t)(1<<11))

Setting this bit Should convert the UART to sending and receiving 7 bits. You should not have to muck with the data going in and out of the FIFO queues.

I have not looked at the T3.x reference manuals, but I would expect similar bit defined somewhere. Assuming they support 7 bit mode.

Hope that helps
 
Here's a summary of what the issue was:

Serial.read() was returning some bytes with the high bit set. It turns out that this was the parity bit. I modified the PR a short time ago to do these things:
1. Define a mask that gets set to 0x7f if the mode is SERIAL_7E1 or SERIAL_7O1.
2. Upon data reception, the byte is AND'ed with the appropriate mask: the 7-bit one or the default one.
3. In an older version of the PR, I also AND'ed the output. It turns out I didn't need to do that so I removed that part. Only the received bytes need to be AND'ed with the mask.
4. Note: I'm using a mask variable instead of if's everywhere to avoid branching if testing for 7-bit mode all the time because I think that's a little faster.

Notes:
In the UART and LPUART chip specs, the "data byte" includes everything except the start and stop bits. That means parity is also included in that byte. So technically, that M7 bit, if using parity, will cause a 6-bit byte plus a parity bit. i.e. setting M7 is the wrong thing to do here.

I'm happy with the PR, so it's ready for review: https://github.com/PaulStoffregen/cores/issues/665

Here's some simple test code:
Code:
void setup() {
  Serial.begin(115200);
  while (!Serial && millis() < 4000) {
    // Wait for Serial
  }

  Serial5.begin(19200, SERIAL_7E1);
}

void loop() {
  int c = Serial5.read();
  if (c >= 0) {
    // c &= 0x7f;  // <-- This line is needed without the fixes
    Serial.printf("c = %02xh\r\n", c);
  }
}

I've tested this with a 7E1 device with both a Teensy 3.2 and a Teensy 4.1. The fixes in the PR work for me and the code does not work as expected if I remove those fixes. i.e. some received bytes will have the high bit set; the value of this bit is always exactly what the parity should be.
 
Just so it’s clear, there is unexpected behaviour for 7E1 and 7O1 modes: the user needs to do a `&0x7f`step after reading a byte or bytes because the system puts the parity bit in the top bit. Respectfully, @KurtE’s answer above isn’t relevant and isn’t related to the issue. (Specifically, the issue isn’t about a ”7N1” mode, and second, setting the M7 bit isn’t correct. I don’t think the issue was tested or understood in that post.)

Now, it may be the case that it is desired that the API requires the `&0x7f` step for every byte, but it’s not documented anywhere and surprised me when I talked to a 7E1 device. Additionally, because there’s a smidgen more processing, and because it’s possible that 7-bit parity modes aren’t as common, it may be the case that the preferred approach is to document this behaviour instead of fixing it in the serial driver.

I'll add: It's fairly easy to do a `&0x7f` for `read()` calls, however, it's a little more tedious when having to, after reading an array of bytes, then go through and do that step for every byte.

I just want to be sure the powers that be don’t dismiss this. Here’s the PR that fixes the issue for Teensy 3 (and LC) and Teensy 4:
https://github.com/PaulStoffregen/cores/pull/665

P.S. I think the title for this thread is misleading. In retrospect, it should have mentioned that extra steps are required to remove the parity bit from the data.
 
Last edited:
<First response removed by self> ;)

Hopefully relevant.

Note: it is not the system that is adding on the parity or the like to the data that is received. It simply returning the data as sent to you by the device. And it is that data that is defined by the protocol.
7E1 or 7O1 - sends 8 bits of data, where the low 7 bits is your information and upper bit is the parity bit. (Luckily, we don't have to support MARK or SPACE)
https://en.wikipedia.org/wiki/Serial_port#Parity

Which I totally agree, can be a pain. I remember some system I worked on long ago, where the code had to generate the parity bit, before it would allow you to send the data.
Likewise, a pain to have to remove it.

As for the Pull Requests - Paul(PRJC) owns the code and obviously he can choose to take it in or not.
My check of the first file, I would not recommend it in the current form, as it will bust other functionality. For example, suppose I had turned on 9 bit mode which we don't enable by default as it doubles the size of the software queues. The updated code ands the data that is placed in the queue by either 0x7f (for your case) or 0xff which would bust the 9 bit data. Or 10 bit data in the case of T4.x.

Also I have no idea if it will bust other developers code, which maybe checks the parity of the bytes coming in against the serial specification.

Now if it were me, and I did not like to have my code have to do the and of the data, I would simply generate a simple wrapper class for the hardware Serial object, that did it for me, while isolating the changes to just my code.

Code:
class RemoveParityHardwareSerial: public HardwareSerial
{
public:
	constexpr RemoveParityHardwareSerial(HardwareSerial &hserial) : _hserial(hserial) {};
	void begin(uint32_t baud, uint16_t format=0) {_hserial.begin(baud, format); }
	void end(void) {_hserial.end();}

	virtual int available(void) {return _hserial.available();
	virtual int peek(void) {return _hserial.peek() & 0x7f; }
	virtual void flush(void) {return _hserial.flush();}
	virtual size_t write(uint8_t c) { return _hserial.write(c);}
	virtual int read(void) {return _hserial.read() & 0x7f); }
	int availableForWrite(void) {return _hserial.availableForWrite(); }
protected: 
        HardwareSerial _hserial;
};

How complete the class needs to be depends on your needs. Also did not try to compile as typed here on the fly, so probably edit issues and/or more methods
that are needed.

But then: you could have something in your code that looks like:
RemoveParityHardwareSerial Serial1NP(Serial1);
And then simply use it instead of Serial1... You can also continue to call Serial1 methods if needed to fill in some things, maybe not in this simple wrapper.

Again if it were me, that is how I would do.
 
@KurtE: First, thanks for engaging and reading the PR. However, I don't agree with your points. Let me elaborate:

Point 1 — 9-bit and 10-bit support:

My PR does still support 9 and 10 bits. First, for teensy3, the `use9Bits` variable is set according to bit 0x80 of the format. None of the 7-bit modes has this set. If you read the PR more carefully, you'll see that the new `data_mask` only gets used when `use9Bits` is false. This is the case for all 7-bit formats. For teensy4, note that for all non-7-bit modes, the mask is set to 0x3ff.

Summary: My PR works for all formats and bit modes in the same way as before.

Point 2 — that `RemoveParityHardwareSerial` wrapper class for removing a parity bit:

I think this proposal confuses the layers of the API. Parity is supposed to be checked by the system, and the user retrieves only the data bits. In other words, it's not appropriate for this API to say "sometimes you have to mask off the parity bit and sometimes you don't." That's a bad inconsistency. As a user, when I call Stream.read() or any of the multi-byte read functions (in whatever API), I expect it to return (or fill in) only data bits/bytes, and not parity bits. I shouldn't have to worry that sometimes, for some formats, I have to mask off the parity bit, but for other formats I don't have to mask off the parity bit. If 7-bit mode requires me to know to mask the parity bit, then so should the other formats. Contra-positively, if other modes don't need to mask off the parity bit, then 7-bit mode shouldn't either.

Summary: This leads to an inconsistent and unexpected API and behaviour because only the 7-bit formats return the parity bits as data bits.

Point 3 — Breaking existing code:

For code that doesn't use 7-bit mode, it's not checking for parity because it can't: the parity bits are never returned in the data. For code that does use 7-bit mode, it's likely that the developer already discovered the data-contains-the-parity-bit problem and is already masking with 0x7f. This brings me to my question for you: Can you give an example where you think existing code would break? I can't think of any, with the exception of code that expects the parity bit to be there in the data bits, but only for 7-bit mode.

Summary: I don't think this will break existing code because the Serial API doesn't promise to return the parity bits in the data. This is, at best, unexpected behaviour.

Please read my PR more carefully, and I think you'll see that it behaves correctly for all serial formats. Of course, if you do find a bug, let me know and I'll fix it. (Thanks again for engaging.)

Update: If only the 7-bit mode requires masking off the 7-bit mode, and Paul decides not to change anything, then at the very least, this should be documented. For example: "For 7-bit mode with parity, if you're just interersted in the data bits, then you must apply the mask 0x7f for every byte received."

Update 2: Another question one could ask is: Why are parity bits only returned for 7-bit mode and not the other modes? This is strange and violates consistency.
 
Last edited:
Sorry, at this point I am going to step back from this and agree to disagree on some of the points.

If Paul agrees to the changes and asks for me to take a look again, I would be happy to do so.

Good luck.
 
Curious, which parts do you agree with, and which do you disagree with? Or are you merely saying that having the parity bit in the returned data is expected? If it’s expected, why doesn’t 8O1 or 8E1 return a 9-bit value? i.e. 7E1 is 8 bits, so 8E1 should be 9 bits, by your logic.

All I’m saying is that if something like 8E1 and 8O1 only return the data bits, then 7E1 and 7O1 should only return the data bits as well.
 
Actually, 8O1 and 8E1, return 9 bits, but I bet you have not enabled 9 bit data... We probably should disable those when 9 bit option is not defined.

Simple program:

Code:
typedef struct {
  uint8_t format;
  const char *format_name;

} formats_t;

formats_t formats_list[] = {
  { SERIAL_7E1, "SERIAL_7E1" },
  { SERIAL_7O1, "SERIAL_7O1" },
  { SERIAL_8N1, "SERIAL_8N1" },
  { SERIAL_8E1, "SERIAL_8E1" },
  { SERIAL_8O1, "SERIAL_8O1" },
  { SERIAL_9N1, "SERIAL_9N1" },
  { SERIAL_9E1, "SERIAL_9E1" },
  { SERIAL_9O1, "SERIAL_9O1" }
};



void setup() {
  while (!Serial)
    ;

  int ch;
  for (uint8_t i = 0; i < (sizeof(formats_list) / sizeof(formats_list[0])); i++) {
    Serial.print("\nTest format: ");
    Serial.println(formats_list[i].format_name);
    Serial1.begin(1000000, formats_list[i].format);
    Serial1.print("ABCDEFGHIJKLMNOP");

    elapsedMillis em = 0;
    for (;;) {
      ch = Serial1.read();
      if (ch != -1) {
        Serial.printf("%03x(%c) ", ch, ch & 0x7f);
        em = 0;
      } else if (em > 100) {
        break;
      }
    }
    Serial.println();
    Serial1.end();
  }
}

void loop() {
}

I jumpered pin 0 to pin 1 on T4.1 and:
Output:
Code:
Test format: SERIAL_7E1
041(A) 042(B) 0c3(C) 044(D) 0c5(E) 0c6(F) 047(G) 048(H) 0c9(I) 0ca(J) 04b(K) 0cc(L) 04d(M) 04e(N) 0cf(O) 050(P) 

Test format: SERIAL_7O1
0c1(A) 0c2(B) 043(C) 0c4(D) 045(E) 046(F) 0c7(G) 0c8(H) 049(I) 04a(J) 0cb(K) 04c(L) 0cd(M) 0ce(N) 04f(O) 0d0(P) 

Test format: SERIAL_8N1
041(A) 042(B) 043(C) 044(D) 045(E) 046(F) 047(G) 048(H) 049(I) 04a(J) 04b(K) 04c(L) 04d(M) 04e(N) 04f(O) 050(P) 

Test format: SERIAL_8E1
041(A) 042(B) 143(C) 044(D) 145(E) 146(F) 047(G) 048(H) 149(I) 14a(J) 04b(K) 14c(L) 04d(M) 04e(N) 14f(O) 050(P) 

Test format: SERIAL_8O1
141(A) 142(B) 043(C) 144(D) 045(E) 046(F) 147(G) 148(H) 049(I) 04a(J) 14b(K) 04c(L) 14d(M) 14e(N) 04f(O) 150(P) 

Test format: SERIAL_9N1
041(A) 042(B) 043(C) 044(D) 045(E) 046(F) 047(G) 048(H) 049(I) 04a(J) 04b(K) 04c(L) 04d(M) 04e(N) 04f(O) 050(P) 

Test format: SERIAL_9E1
041(A) 042(B) 243(C) 044(D) 245(E) 246(F) 047(G) 048(H) 249(I) 24a(J) 04b(K) 24c(L) 04d(M) 04e(N) 24f(O) 050(P) 

Test format: SERIAL_9O1
241(A) 242(B) 043(C) 244(D) 045(E) 046(F) 247(G) 248(H) 049(I) 04a(J) 24b(K) 04c(L) 24d(M) 24e(N) 04f(O) 250(P)

But again, I turned on 9 bit mode.
 
Quick follow on to yesterday's post:

Updated test sketch:

Code:
typedef struct {
  uint32_t format;
  const char *format_name;
  uint16_t high_limit_test;
} formats_t;

formats_t formats_list[] = {
  { SERIAL_7E1, "SERIAL_7E1", 128 },
  { SERIAL_7O1, "SERIAL_7O1", 128 },
  { SERIAL_8N1, "SERIAL_8N1", 256 },
  { SERIAL_8E1, "SERIAL_8E1", 256 },
  { SERIAL_8O1, "SERIAL_8O1", 256 },
#ifdef SERIAL_9N1
  { SERIAL_9N1, "SERIAL_9N1", 512 },
  { SERIAL_9E1, "SERIAL_9E1", 512 },
  { SERIAL_9O1, "SERIAL_9O1", 512 }
#endif
};
#ifdef STM32F4xx
#define HSERIAL Serial3
#endif
#ifndef HSERIAL
#define HSERIAL Serial1
#endif


void setup() {
  while (!Serial)
    ;
  Serial.begin(115200);
  int ch;
  for (uint8_t i = 0; i < (sizeof(formats_list) / sizeof(formats_list[0])); i++) {
    Serial.print("\nTest format: ");
    Serial.print(formats_list[i].format_name);
    HSERIAL.begin(1000000, formats_list[i].format);

    Serial.print(" Write time us: ");
    uint32_t time_start = micros();
    HSERIAL.print("ABCDEFGHIJKLMNOP");
    HSERIAL.flush();
    Serial.println(micros() - time_start, DEC);

    time_start = millis();
    for (;;) {
      ch = HSERIAL.read();
      if (ch != -1) {
        Serial.print(ch, HEX);
        Serial.print("(");
        Serial.write(ch & 0x7f);
        Serial.print(") ");
        time_start = millis();
      } else if ((millis() - time_start) > 100) {
        break;
      }
    }

    // try a few higher numbers;
#ifdef SERIAL_9N1
    for (ch = formats_list[i].high_limit_test - 5; ch < formats_list[i].high_limit_test; ch++) HSERIAL.write9bit(ch);
#else
    for (ch = formats_list[i].high_limit_test - 5; ch < formats_list[i].high_limit_test; ch++) HSERIAL.write(ch);
#endif
    Serial.print(" : ");
    time_start = millis();
    for (;;) {
      ch = HSERIAL.read();
      if (ch != -1) {
        Serial.print(ch, HEX);
        Serial.print(" ");
        time_start = millis();
      } else if ((millis() - time_start) > 100) {
        break;
      }
    }
Serial.println();
    HSERIAL.end();
  }
}

void loop() {
}

Output on T4.1 with 9 bit serial enabled.

Code:
Test format: SERIAL_7E1 Write time us: 170
41(A) 42(B) C3(C) 44(D) C5(E) C6(F) 47(G) 48(H) C9(I) CA(J) 4B(K) CC(L) 4D(M) 4E(N) CF(O) 50(P)  : 7B FC 7D 7E FF 

Test format: SERIAL_7O1 Write time us: 171
C1(A) C2(B) 43(C) C4(D) 45(E) 46(F) C7(G) C8(H) 49(I) 4A(J) CB(K) 4C(L) CD(M) CE(N) 4F(O) D0(P)  : FB 7C FD FE 7F 

Test format: SERIAL_8N1 Write time us: 171
41(A) 42(B) 43(C) 44(D) 45(E) 46(F) 47(G) 48(H) 49(I) 4A(J) 4B(K) 4C(L) 4D(M) 4E(N) 4F(O) 50(P)  : FB FC FD FE FF 

Test format: SERIAL_8E1 Write time us: 188
41(A) 42(B) 143(C) 44(D) 145(E) 146(F) 47(G) 48(H) 149(I) 14A(J) 4B(K) 14C(L) 4D(M) 4E(N) 14F(O) 50(P)  : 1FB FC 1FD 1FE FF 

Test format: SERIAL_8O1 Write time us: 188
141(A) 142(B) 43(C) 144(D) 45(E) 46(F) 147(G) 148(H) 49(I) 4A(J) 14B(K) 4C(L) 14D(M) 14E(N) 4F(O) 150(P)  : FB 1FC FD FE 1FF 

Test format: SERIAL_9N1 Write time us: 188
41(A) 42(B) 43(C) 44(D) 45(E) 46(F) 47(G) 48(H) 49(I) 4A(J) 4B(K) 4C(L) 4D(M) 4E(N) 4F(O) 50(P)  : 1FB 1FC 1FD 1FE 1FF 

Test format: SERIAL_9E1 Write time us: 205
41(A) 42(B) 243(C) 44(D) 245(E) 246(F) 47(G) 48(H) 249(I) 24A(J) 4B(K) 24C(L) 4D(M) 4E(N) 24F(O) 50(P)  : 1FB 3FC 1FD 1FE 3FF 

Test format: SERIAL_9O1 Write time us: 205
241(A) 242(B) 43(C) 244(D) 45(E) 46(F) 247(G) 248(H) 49(I) 4A(J) 24B(K) 4C(L) 24D(M) 24E(N) 4F(O) 250(P)  : 3FB 1FC 3FD 3FE 1FF

The sketch was updated to run on a few more different boards. And yes at least some of these either the hardware or the software appears to mask off the parity bit.
I know I have played with some others that did not. So I am personally neutral here on this.

The T4.x code looks fine, although I don't think it is catching the other cases of it like 8N1/8E1, 9N1, 9E1, the mask would also need to change. In the non-parity case not sure you need to set to 0x3ff, could be 0xffff.

My only main concern was compatibility with existing code. That is, some of this code has been working this way for many years now. Like first pull into cores on github for T3.x was going on 10 years ago.
Are there code bases out there, who look at the parity bit to verify that the data that they received was valid or not? I don't believe for example that we have code in place that enables the interrupt on parity error or the like.

So might there be code that does a quick bit count on each byte (vcnt?) and verify correct? I don't know.

Likewise, if someone sets up a teensy as a USB to serial adapter, and have it forward the data from the UART to the USB port, would something on the host (PC or Linux...) also be setup to check the parity of the data returned? Again, I don't know.

But I do understand the convivence of not having to mask the data.

Good luck
 
It seems to me the most sensible option is to expand the serial mode (cores/teensy4/HardwareSerial.h) with a mask flag, bit 6 or bit 10, that when set, causes the data to be masked to contain only the data bits. Of course, masking would always be done, and the bit just select what its value would be. The type should be BUFTYPE, defaulting to ~(BUFTYPE)0. Examining the config bits one can trivially tell what the mask should be when masking is actually requested. Perhaps add the corresponding SERIAL_???_DATAONLY config constants, too.

Put the actual mask (definition, initialization, and use) behind SERIAL_MASKING_SUPPORT, and those who don't want the extra cost of masking won't see it, for the cost of a bit of #ifdef SERIAL_MASKING_SUPPORT ... #endif'ery in the sources.
 
Back
Top