Expose CDC settings to sketch

[matthijskooijman]

Having these settings available to the sketch, allows turning the Leonardo into a proper USB->serial converter (without having to hardcode the baudrate), to allow e.g. a computer to talk to an XBee module.

[PaulStoffregen]

Please resist the urge to use weak linked symbols as official published APIs.

We already have serialEvent() as an example. Libraries that take a Stream or top-level class reference or pointer can't use serialEvent(), because the actual function name has to be used in the code.

A common pitfall for novices trying to use serial events for Serial1, Serial2 or Serial3 involves getting the number in the wrong place in the function name. There's no compiler checking for the name or args or return types, because the function isn't declared in a header. For these and other reasons, weak linker symbols are generally a poor practice for public APIs.

[matthijskooijman]

Good point. Having a proper callback is probably better, at the cost of two bytes of RAM, which seems acceptable. Making it more general might be a good idea then as well, having a callback that gets called on all setup packets?

How would this function be named? void onUsbSetup(bool *func(const Setup& setup)) or perhaps attachUsbSetup()?

Should the callback function return a bool and how should it be handled? Should returning true cause CDC_Setup() to return true as well? Should it cause CDC_Setup() to skip remaining processing? Should no value be returned (causing e.g. the break command to return a Stall to the USB host, not sure what the results of that would be). I'd propose a return value of true to mean that CDC_Setup should return true, but still do its own processing regardless of the callback return value?

[matthijskooijman]

I just updated this PR, replacing the SEND_BREAK weak function with a more general onUsbSetupPacket() callback function (which can be used for more than just handling break messages). @PaulStoffregen, does this look better to you too?

[PeterVH]

Isn't this api (a bit) too low level? It is on the USB level rather than on the level of a serial line.
An api on the serial line level could be:

    void lineStateEvent(void);   // RTS or DTR change
    bool lineCodingEvent(void);   // user changes baud rate,...

Should the callback function return a bool and how should it be handled? Should returning true cause CDC_Setup() to return true as well? Should it cause CDC_Setup() to skip remaining processing?

I would rather see the callbacks as a notification of the sketch and not let the sketch impact the driver behaviour more than necessary. More specifically:

lineStateEvent() returns void: if the line state changes the sketch cannot/need not do anything about it.
lineCodingEvent() returns bool so it can e.g. reject a baud rate.

I like your idea of letting the callbacks take no arguments: baud rate and friends can be retrieved via your getters.

[PaulStoffregen]

does this look better to you too?

Well, better is relative. Weak linking should usually be avoided for published APIs, so in that respect, yes, better.

But whether it's a good idea to add interrupt context callbacks as an Arduino API is another question! Even experts have a tough time properly dealing with the pitfalls of interrupt context. At the very least, good documentation and examples that really do show volatile variables and proper use of noInterrupts() should be written.

Exposing very low-level USB details should also be considered carefully. Just because you can do a thing does not necessarily mean you should do that thing.

The non-callback stuff that corresponds to well known serial parameters look good to me.

[matthijskooijman]

Isn't this api (a bit) too low level? It is on the USB level rather than on the level of a serial line.

You are right that the callback is very low-level. Having multiple higher-level callbacks would be better, except that figuring out the list of methods we'd need is tricky. This would likely cost a lot of effort for some features that won't be used much and mostly by advanced users. Even if we come up with a proper API, it is likely that we'll miss a usecase. Even more, every callback takes up 2 bytes of RAM, even when it is unused.

For these reasons, I think that a single low-level callback might be better. It enables a lot of usecases, against a minimal cost (both in terms of engineering time and RAM overhead).

I like your idea of letting the callbacks take no arguments: baud rate and friends can be retrieved via your getters.

I think that was your proposal?

Exposing very low-level USB details should also be considered carefully. Just because you can do a thing does not necessarily mean you should do that thing.

Agreed. Of course, the reason for doing so is, in this case, that I need to forward SEND_BREAK requests from the CDC side to the UART side (and possibly also get notified about set line control and line state requests, though it seems I can get by with polling this for now).

[PeterVH]

I like your idea of letting the callbacks take no arguments: baud rate and friends can be retrieved via your getters.

I think that was your proposal?

I realize I made a reasoning step I did not explain: if e.g. the baud rate gets changed, your on_cdc_setup_packet(const Setup&) callback does not pass the new baud rate: the sketch needs to get to get that using your getter (the baud rate is not in the Setup packet, it is in the 7 byte payload).

Having read Paul's post I realize that working without callbacks may be a good option. You may have seen my write up about using the Leonardo as a serial converter. I think I can implement that use case, just with the getters: the sketch can check for a baud rate change or a line state change from the arduino loop(). The use case involves setting the uart's baud rate and forwarding DTR to an arduino pin. There may be a need for a line state change api in order not to miss a DTR drop pulse though.

I never went further and created a change request to the arduino developers (like you do now) because I was not comfortable about the api and found nobody to exchange ideas with. Now Paul already ironed out the weak symbols (I used them too) and confirms the concerns I had about callbacks in irq mode.

I don't understand very well (because I never used it myself) the use case for the SEND_BREAK. Do you need it for the xbee device you want to work with?

To have an idea of what feature set to implement, I looked in the linux kernel to see what they do. The (device side) linux acm driver does not handle SEND_BREAK either. Basically they implement the same feature set as in the arduino core: line state and line encoding get/set. (cfr. drivers/usb/gadget/function/f_acm.c)

Well. Whatever you come up with, I'll be happy to test it with the use case mentioned above.

[PaulStoffregen]

This may or may not help, but Teensy has long supported some of these features (from before Leonardo was created). The CDC baud rate is accessed with Serial.baud(). No complex callbacks or very low-level USB details are exposed to sketches.

Here's the USB serial converter sketch for Teensy.

/* USB to Serial - Teensy becomes a USB to Serial converter
   http://dorkbotpdx.org/blog/paul/teensy_as_benito_at_57600_baud

   You must select Serial from the "Tools > USB Type" menu

   This example code is in the public domain.
*/

// set this to the hardware serial port you wish to use
#define HWSERIAL Serial1

unsigned long baud = 19200;
const int reset_pin = 4;
const int led_pin = 13;  // 13 = Teensy 3.X & LC
                         // 11 = Teensy 2.0
                         //  6 = Teensy++ 2.0
void setup()
{
  pinMode(led_pin, OUTPUT);
  digitalWrite(led_pin, LOW);
  digitalWrite(reset_pin, HIGH);
  pinMode(reset_pin, OUTPUT);
  Serial.begin(baud);       // USB, communication to PC or Mac
  HWSERIAL.begin(baud);     // communication to hardware serial
}

long led_on_time=0;
byte buffer[80];
unsigned char prev_dtr = 0;

void loop()
{
  unsigned char dtr;
  int rd, wr, n;

  // check if any data has arrived on the USB virtual serial port
  rd = Serial.available();
  if (rd > 0) {
    // check if the hardware serial port is ready to transmit
    wr = HWSERIAL.availableForWrite();
    if (wr > 0) {
      // compute how much data to move, the smallest
      // of rd, wr and the buffer size
      if (rd > wr) rd = wr;
      if (rd > 80) rd = 80;
      // read data from the USB port
      n = Serial.readBytes((char *)buffer, rd);
      // write it to the hardware serial port
      HWSERIAL.write(buffer, n);
      // turn on the LED to indicate activity
      digitalWrite(led_pin, HIGH);
      led_on_time = millis();
    }
  }

  // check if any data has arrived on the hardware serial port
  rd = HWSERIAL.available();
  if (rd > 0) {
    // check if the USB virtual serial port is ready to transmit
    wr = Serial.availableForWrite();
    if (wr > 0) {
      // compute how much data to move, the smallest
      // of rd, wr and the buffer size
      if (rd > wr) rd = wr;
      if (rd > 80) rd = 80;
      // read data from the hardware serial port
      n = HWSERIAL.readBytes((char *)buffer, rd);
      // write it to the USB port
      Serial.write(buffer, n);
      // turn on the LED to indicate activity
      digitalWrite(led_pin, HIGH);
      led_on_time = millis();
    }
  }

  // check if the USB virtual serial port has raised DTR
  dtr = Serial.dtr();
  if (dtr && !prev_dtr) {
    digitalWrite(reset_pin, LOW);
    delayMicroseconds(250);
    digitalWrite(reset_pin, HIGH);
  }
  prev_dtr = dtr;

  // if the LED has been left on without more activity, turn it off
  if (millis() - led_on_time > 3) {
    digitalWrite(led_pin, LOW);
  }

  // check if the USB virtual serial wants a new baud rate
  if (Serial.baud() != baud) {
    baud = Serial.baud();
    if (baud == 57600) {
      // This ugly hack is necessary for talking
      // to the arduino bootloader, which actually
      // communicates at 58824 baud (+2.1% error).
      // Teensyduino will configure the UART for
      // the closest baud rate, which is 57143
      // baud (-0.8% error).  Serial communication
      // can tolerate about 2.5% error, so the
      // combined error is too large.  Simply
      // setting the baud rate to the same as
      // arduino's actual baud rate works.
      HWSERIAL.begin(58824);
    } else {
      HWSERIAL.begin(baud);
    }
  }
}

[matthijskooijman]

I think I can implement that use case, just with the getters: the sketch can check for a baud rate change or a line state change from the arduino loop().

Yes, for a basic version, just polling the getters is sufficient (in fact, that's what I'm doing in my sketch as well, since I can't handle baud changes in an ISR, so setting a flag and polling that isn't really a lot easier, perhaps only a bit faster, than polling the accessors).

I don't understand very well (because I never used it myself) the use case for the SEND_BREAK. Do you need it for the xbee device you want to work with?

When talking to XBee devices and doing firmware upgrades or recovery, breaks can be useful. A break essentially means to pull the TX pin low for > 1 byte time. The XCTU tool uses this to pull DIN (the XBee's RX pin) low. Some of the XBee modules will stick in the bootloader if DIN is low at boot time (usually combined with some state of DTE and/or RTS).

Ideally, I'd also be able to sample CTS and report that back to the host, but it turns out that there really is no way for a CDC device to report CTS through USB - it's simply not part of the protocol...

[PeterVH]

I see. Break detection can indeed be a useful feature. Out of curiosity, I tried out an FTDI serial converter: not too surprisingly it forwards breaks correctly to its tx line. So an usb2serial sketch should be able to do that too.

We could leave it to sketches to implement that using your on_cdc_setup_packet() callback as a hook to extend or customize the driver.

But for break detection it equally makes sense to implement an api for direct use in sketches. Something like:

    bool breakDetected();
    void clearBreakDetected();

B.t.w., the best way to document/explain/motivate this feature request is to come up with a sketch like Paul's that uses the new api's to build a complete, usable usb2serial converter.

[matthijskooijman]

bool breakDetected();
void clearBreakDetected();

Not sure if these would do the trick, since a "break request" from the USB host includes a break time (or, can specify that the break should be maintained until explicitely cleared by the USB host). Or perhaps I'm misunderstanding how you wanted these two methods to behave.

B.t.w., the best way to document/explain/motivate this feature request is to come up with a sketch like Paul's that uses the new api's to build a complete, usable usb2serial converter.

Excellent point. I already have that, so here it is:

unsigned long breakEnd = 0;

#define CDC_SEND_BREAK 0x23
bool onUsbSetupPacket(const Setup &setup)
{

  if (setup.bmRequestType ==REQUEST_HOSTTODEVICE_CLASS_INTERFACE &&
      setup.bRequest == CDC_SEND_BREAK) {
    digitalWrite(13, HIGH);
    uint16_t duration = ((uint16_t)setup.wValueH << 8) | setup.wValueL;
    // Force the TX pin low and then disable the UART TX side so the
    // digitalWrite actually takes effect
    UCSR1B &= ~(1 << TXEN1);
    digitalWrite(1, LOW);
    if (duration == 0xffff) { // Indefinitely
      breakEnd = 0;
    } else {
      // duration = 0 means "end break now"
      breakEnd = millis() + duration;

      // Prevent problems if the break happens shortly before a millis()
      // overflow (at the expense of 1ms extra break)
      if (!breakEnd)
    breakEnd++;
    }
  }
  return true;
}

void setup() {
  // Force TX high, so it will remain high before we configure Serial1
  // and between reconfigurations. Having it as an output also allows
  // sending breaks by disabling TX and writing it low.
  pinMode(1, OUTPUT);
  digitalWrite(1, HIGH);

  // Baudrate is ignored
  Serial.begin(0);
  Serial.onUsbSetupPacket(onUsbSetupPacket);
}

void loop() {
  unsigned long baud = Serial.baudrate();
  Serial1.begin(baud);
  while(Serial.baudrate() == baud) {
    pinMode(2, Serial.dtr() ? OUTPUT : INPUT);
    pinMode(3, Serial.rts() ? OUTPUT : INPUT);
    if (breakEnd) {
      if ((long)(millis() - breakEnd) >= 0) {
    UCSR1B |= (1 << TXEN1);
    digitalWrite(1, HIGH);
    breakEnd = 0;
      }
    } else {
      if (Serial.available())
    Serial1.write(Serial.read());
    }
    if (Serial1.available())
      Serial.write(Serial1.read());
  }
  Serial1.end();
}

Note that this directly touches the UART registers as well, so if we really want to support this usecase properly, we should also modify HardwareSerial to support sending breaks.

[PaulStoffregen]

I really hope these APIs can be thought out well enough that people aren't driven to accessing AVR-specific UART registers. Quite a lot of that type of code still exists, leftover from the many years before Serial.begin() supported non-8N1 encoding and Serial.availableForWrite() allowed non-blocking transmit. It's a terrible pain when people try to use non-AVR boards, which are becoming far more common lately.

Even though I've said this many times, I'd like to repeat again how difficult interrupt context callbacks are, even for experts. In the code above, "breakEnd" is a 4-byte variable that's read and written in the interrupt, and read and written in the main program. It's not declared volatile, and there's no attempt at assuring atomic access. While these issues can be fixed in examples, when I see such code posted by the most talented and experienced Arduino developers and contributors, I feel very skeptical that novice Arduino users will ever be able to use such APIs without extremely difficult to diagnose troubles.

[matthijskooijman]

In the code above, "breakEnd" is a 4-byte variable that's read and written in the interrupt,

Hm, excellent point. I just pointed out exactly this problem in another pullrequest, but hadn't properly looked at my own code yet :-)

So, if we'd want to turn this into a proper API for break, how would that look? If polling is ok (which I guess would be, at least for this usecase, a single inBreak() or breakActive() would be sufficient, if CDC.cpp would take care of the timing associated here. Perhaps having a breakEndTime() could be good as well (or breakDurationLeft()), to allow different behaviour depending on the break duration?

[PeterVH]

The api should be such that for regular use cases no irqmode callback is needed, right?
So the example sketch should have no callback either.

...CDC.cpp would take care of the timing associated here.

I would not do that. Time would start ticking from the moment you receive the CDC_SEND_BREAK request. This implies you must initiate the desired action from the irq. That is only possible via a callback, which is not what we want.

I would rather have a getter that just returns the last break duration sent (and correctly synchronized with the irq handler of coarse).

The problem with this approach is that if the host sends break(0xffff), and later on stops the break condition by sending break(0), the sketch might never notice the break at all if it was doing a lengthy operation. That is where the duo breakDetected() clearBreakDetected() could come in. After break(0) comes in, breakDuration() wil return 0, but breakDetected() will still return true untill clearBreakDetected() is called.

Alternatively, this situation could be simply ignored (but what is the point in creating an api that works most of the time?) . In Paul's sketch there is a similar possibility (at least theoretically) that the dtr() could toggle without the sketch noticing.

I really hope these APIs can be thought out well enough that people aren't driven to accessing AVR-specific UART registers.

The most logical thing to do about this is add sendBreak() to HardwareSerial. We could also start out with a simple Serial.end()...

[PeterVH]

...leftover from the many years before Serial.begin() supported non-8N1 encoding and Serial.availableForWrite() allowed non-blocking transmit.

Speaking of which: Serial_ has no availableForWrite() yet...

[matthijskooijman]

The api should be such that for regular use cases no irqmode callback is needed, right?
So the example sketch should have no callback either.

That's still under discussion, but I mostly posted the sketch that works with the current version of the pullrequest, to get a bit more feeling for the usecase.

I would not do that. Time would start ticking from the moment you receive the CDC_SEND_BREAK request. This implies you must initiate the desired action from the irq. That is only possible via a callback, which is not what we want.

Ah, good point. So, a scheme where the break duration is returned only once seems better, then.

The problem with this approach is that if the host sends break(0xffff), and later on stops the break condition by sending break(0), the sketch might never notice the break at all if it was doing a lengthy operation.

That's true, but one might wonder if there is still any value in interpreting a break even after it has been lifted. It does seem elegant to offer the choice to the sketch, but I'm not sure if this is possible without a signifcantly more complicated API? Ideally, yu'd keep a list of events, along with timestamps, which can be read by the sketch in order (including e.g. dtr-on / dtr-off events, to fix the missed dtr issue). Of course, this opens an entirely new can of worms regarding complexity and memory allocation, which I think we'll not want to get into.

I'm not entirely sure if I understand your proposal. I think you're saying that breakDetected() returns true if a break message was received since the last call to clearBreakDetected() and breakDuration() always returns the most recent duration received? In the 0xffff and then 0x0 scenario, this would mean that the sketch will see the 0x0 break message, which tells it there was likely a 0xffff before?

If so, I guess that making the clearBreakDetected() implicit whenever breakDuration() is called might be better? This looks a bit more like the available() / read() API. Perhaps renaming to breakAvailable() and readBreak() might be useful? Then breakAvailable() returns true when a break message was received since the last readBreak() and readBreak() always returns the last duration? For perfect symmetry, readBreak should return a long and return -1 when no new break message was received, which might actually remove the need for breakAvailable() entirely?

[PeterVH]

For perfect symmetry, readBreak should return a long and return -1 when no new break message was received, which might actually remove the need for breakAvailable() entirely?

Yes that's it! The readBreak() is all we need if -1 indicates there is no break.
Hehe, I found it surprisingly difficult to find something elegant for a seemingly simple problem. That 0xffff 0 construction confused things....

Btw, I think after a 'break 0', readBreak() should return 0, not -1. This indeed hints the sketch there has probably been a 'break 0xffff, break 0' sequence that it missed. It might decide to send a break (of some arbitrary length (say 2 byte times)) to the uart anyway.

Now the breakAvailable() is not there anymore, I would not call the getter readBreak() anymore, rather something like breakDuration() or getBreak().

Another possible attention point: I hope in sketches the 0xffff does not get widening casted to a value of -1. (It normally does not since 0xffff is unsigned. Only a bug like this would cause a problem:

    int x = readBreak();  // bug: readBreak() returns long
    if (x == 0xffff)
        // indefinite break started when there really is no break

and it will give a compiler warning: signed-unsigned comparison)

[matthijskooijman]

Another possible attention point: I hope in sketches the 0xffff does not get widening casted to a value of -1. (It normally does not since 0xffff is unsigned. Only a bug like this would cause a problem:

Looking at the rules for literals, I think this should be ok. 0xffff will end up as a positive long literal, which does not need to be extended further (and if it would, it would be and stay positive).

So, the proposed API is a single getBreak() function that:

• When a break message was received since the last call to getBreak(), returns the duration of the most recently received message.
• When no break message was received since the last call to getBreak(), returns -1.

Did I get that right? @PaulStoffregen & @cmaglie, does that sound ok to you?

[PeterVH]

Did I get that right?

Yes.

I did some testing with the first part of the pull request: I used Paul's sketch verbatim.
I could upload the AsciiTable sketch to a stand alone avr (with the duemilanova bootloader).
I could receive output from the running sketch in the serial monitor.
(so this functionally tests baud rate change, dtr(), and moving data in both ways)

I noticed the sketch uses baud() whereas your pull request has baudrate(). For people having both teensies and arduino's it may be better to stay compatible with the teensy library?

I added Serial_.availableForWrite(). I guess I'll have to make a separate issue for this? My changes are here (a fork of your branch).

[matthijskooijman]

I noticed the sketch uses baud() whereas your pull request has baudrate(). For people having both teensies and arduino's it may be better to stay compatible with the teensy library?

I like baudrate() marginally better, but compatibility seems more important, so let's use baud().

I added Serial_.availableForWrite(). I guess I'll have to make a separate issue for this? My changes are here (a fork of your branch).

Having a separate pullrequest might make it easier to merge both. Care to create one? This should probably also modify the SAM version of CDC.cpp (which I think is very similar).

[matthijskooijman]

I updated this pullrequest:

• I added the readBreak() API previously discussed.
• I removed the callback API, which was too complicated and the break API removes my need for it.
• I added the _availableForWrite commit by @PeterVH
• I modified a few more method names (databits -> numbits, baudrate -> baud, parity -> paritytype) to match the current Teensy code.
• I added support for SAM to the first commit.
• I fixed a bug in HardwareSerial::end() I noticed with my sketch that changes baudrates.

With this commit, I was able to compile Paul's example unmodified, for the Leonardo. By adding a dummy availableForWrite() and using the proper Serial objects in the sketch, it also worked for the Due (testing the serial data and DTR line using a logic analyzer). I tested the break requests on the Leonardo using this sketch, which can successfully upload new firmware to XBee S1 devices (which needs both break and DTR, verified using the logic analyzer. The sketch contains code for the Due too, but that didn't work, see below.

I also tried adding availableForWrite() and readBreak() for SAM, but that didn't work out. For the former, there is a declaration of USBD_SendSpace(), but its implementation has been commented (because it is broken? dunno...). For the latter, I have an implementation, but I couldn't make it work with my XBee programming setup somehow. I spent a few hours digging around the SAM code and docs and basic breaking using minicom works, but somehow the breaking (using 0xffff and 0x0) that XCTU does will not work. I don't have enough experience with SAM, and no more time right now, to further dig into this, but my attempts are here if anyone else wants to dig in: https://github.com/matthijskooijman/Arduino/tree/cdc-settings-sam

As far as I'm concerned, this PR is ready to be merged. Any thoughts?

[matthijskooijman]

@PaulStoffregen, @PeterVH, does this PR look ok to you now?

[cmaglie]

Looks good to me, it seems to address all the issues.

I'm merging it, let's continue the discussion here if there are more concerns.

[NicoHood]

I want to build an USB-Serial Bridge, similar to paul but now with a leonardo or hoodloader. I also need to reset the mcu with a DTR change to implement a full Arduino-Serial converter sketch.

I just dont understand how to use the break api. May someone add this to the docs?

[matthijskooijman]

@NicoHood The break API is only when you need to send break signals (where TX is pulled low for > 1 byte time), which you likely don't need. The rest of this PR is useful for implementing a converter, by simply polling the dtr() and baud() functions for changes. See the last sketch posted by me above for a full converter including break, or the sketch posted by Paul for a converter without break support.

[PeterVH]

@matthijskooijman : Sorry for the late response. I am glad this is merged: it is great functionality and the api feels good. During my vacation, I already used it on my Leonardo, with Paul's usb to serial sketch. It works great!

[Netoperz]

I'm a bit lost, where exactly in the repo is the sketch code for usb serial converter ?

[matthijskooijman]

@Netoperz, I don't think this is the right place to ask, and I'm not exactly sure what you're asking. If you want to know what part of the code handles the USB communication on a 32u4-based board (like the Leonardo), that code is here: https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/CDC.cpp and here: https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/USBCore.cpp

[Netoperz]

There was a sketch some time ago, that was converting Arduino leonardo in to a USB <==> serial adapter, with dynamic baud rate detection. Some forum posts were pointing here.

I'm looking for that sketch, because on OS X (after resent updates from apple @#$%!^) FTDI chips are not working correctly, even with signed drivers, when the FTDI part to usb bridge is detached and attached to the same USB socket is not recognized until reboot. There is no problem with boards with Atmega32u4, so i figured out, that best solution "on the go" will be to make Arduino Pro Micro or other leonardo to work as a UART to USB converter :) sorry if this is wrong place, i just followed wrong link probably.

[PeterVH]

The sketch you are looking for, is above in Paul Stoffregen's comment.

[matthijskooijman]

@Netoperz, ah, I misunderstood your question then. Indeed, the post @PeterVH has a sketch that I think should work with the current code. In this blogpost, I've published another (slightly more complete IIRC) version of such a sketch (scroll down to "Forwarding through the Arduino Leonardo").

Regarding your problem with FTDI - The current Arduino Uno and Mega boards use a 16u2 microcontroller instead of an FTDI chip, so you might be able to use one of those as well.