patchAVR Downloader/UploaDEr - Patches: patch #7559, 'arduino' programmer type DTR...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #7559: 'arduino' programmer type DTR transition fix

Submitter:  Robert Spitzenpfeil <madworm>
Submitted:  Mon 20 Jun 2011 06:19:17 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Wont Do Privacy:  Public
Assigned to:  joerg_wunsch Open/Closed:  Open

Jump to the original submission

Fri 09 Sep 2011 10:10:37 PM UTC, comment #31: 

@mellis:

I will cook something up for arduino.c (comments only, but they might be extensive).

Robert Spitzenpfeil <madworm>
Wed 24 Aug 2011 03:49:57 PM UTC, comment #30: 


> a) add the comments about the HIGH/LOW business to 'arduino.c' ?


Please do.  I don't have an idea what I'm supposed to
add there ... I reopened the patch tracker item, so you
could add it here easily.

> b) create an updated release file


I'm about to do that, that's why I'm walking down the bug
and patch lists.

Joerg Wunsch <joerg_wunsch>
Group administrator
Wed 24 Aug 2011 03:28:15 PM UTC, comment #29: 

Would it be possible to at least

a) add the comments about the HIGH/LOW business to 'arduino.c' ?

b) create an updated release file (.tar.gz) that includes the latest patches. Always having to drill through the source repository is not funny at all. Patch #7100 is out for quite a while and sadly not part of the 'latest' avrdude-5.10.tar.gz that is available on the download web page.

Robert Spitzenpfeil <madworm>
Wed 24 Aug 2011 02:57:35 PM UTC, comment #28: 

OK, I'm closing it as "won't do" then.  The discussion can
be found both, here in the patch tracker as well as the
mailinglist archives, so hopefully, someone interested in
the details will be able to find it.

Joerg Wunsch <joerg_wunsch>
Group administrator
Wed 24 Aug 2011 02:52:58 PM UTC, comment #27: 

I appreciate your desire to make this as portable as possible, but I don't think we should worry about true RS232 connections for a few reasons:

First, we don't use RS232 for current Arduino boards.  The old serial boards that did use it don't have auto-reset.

Second, it's not clear how the auto-reset is supposed to be triggered for RS232 boards.  I'd argue that the behavior of existing Arduino boards (i.e resetting on a high->low voltage transition of DTR) sets a software standard (i.e. a 0 to 1 logical transition) for triggering the auto-reset.  So if someone does make an Arduino-like board with true RS232, they should base its auto-reset circuitry on this defacto software reset standard - i.e. they should make their board work with the current code. 

Finally, the current code works and I've tested it on multiple operating systems.  Any changes will require retesting on a variety of configurations.  This doesn't seem worth it for hypothetical future boards whose behavior we don't know (and therefore can't test).

David A. Mellis <mellis>
Wed 24 Aug 2011 02:37:35 PM UTC, comment #26: 

Personally I'd like to see something that is completely invariant to whether a real RS232 port/device is connected or a TTL serial adapter, so that it doesn't matter if signal levels are inverted or not. As I had to find out, there doesn't seem to be an easy way for the computer to know which is which.

It should always work. There should be comments that explain what is done and why.

My previously proposed addition of the 'toggle' function would provide just that, BUT a) I'm not capable of providing the necessary patches to 'ser_win32.c' b) it would also require changes to 'ser_posix.c' as well.

So what to do?

If the bootloader timeout is long enough (really don't know about all optiboot versions for sure), 'arduino.c' could be modified as such:

------

  // explanatory comments should go here...

  serial_set_dtr_rts(&pgm->fd, 0);
  usleep(50*1000);
  serial_set_dtr_rts(&pgm->fd, 1);
  usleep(50*1000);
  serial_set_dtr_rts(&pgm->fd, 0);
  usleep(50*1000);
  serial_set_dtr_rts(&pgm->fd, 1);
  usleep(50*1000);

------

This would provide at least one usable (5V/0V) transition for the reset capacitor to be pulled in the right direction, regardless what 'HIGH' actually means.

I could do some reliability testing on homebrew and a Diecimila board using various bootloaders, but only on linux.

Robert Spitzenpfeil <madworm>
Wed 24 Aug 2011 02:16:06 PM UTC, comment #25: 

No changes to the code are needed.  The current (SVN) avrdude correctly resets Arduino boards on upload.

It could be useful to patch the comments to clarify the situation (i.e. that a logical 1 is a physical low level voltage, and 0 is Vcc).  Robert, do you want to submit such a patch?

David A. Mellis <mellis>
Wed 24 Aug 2011 02:02:49 PM UTC, comment #24: 

Can someone summarize for me the action to take?

As I don't have an Arduino around, I'd only like to commit
something that makes everybody happy.

Joerg Wunsch <joerg_wunsch>
Group administrator
Mon 25 Jul 2011 06:07:50 AM UTC, comment #23: 

There was a hardware bug in my 2nd board, so after repairing that both boards upload and reset equally well with your proposed toggle patches Robert.

Bernard Johnson <bjohnson>
Tue 19 Jul 2011 10:27:10 AM UTC, comment #22: 

There are level shifter ICs... as you know very well. And e.g. the MAX232 doesn't translate more than RX/TX.

But maybe we should forget about RS-232. Nobody with a sane mind uses that anymore (industrial stuff excluded for now).

And whoever thought that using a negative voltage for logic 1 is a good thing should not come anywhere near me.


Robert Spitzenpfeil <madworm>
Tue 19 Jul 2011 01:20:07 AM UTC, comment #21: 

If someone hooked up an AVR to a real RS-232 interface, the +-12V would probably blow it up.  So I think the code is fine.

You're right, though, that the comment in arduino.c is confusing and, I think, wrong.  It's probably worth submitting / applying a patch for that.

David A. Mellis <mellis>
Mon 18 Jul 2011 06:40:49 PM UTC, comment #20: 

And another one...

Now what happens if someone should connect an AVR chip to a real RS-232 interface ?

As the levels for DTR seem to be inverse there, the transition for resetting the arduino will be wrong as well. (just like before patch #7100 for 5V systems).

Therefore I suggest to use something similar to comment #4 / file #23663. This will work independently of which way around the voltage levels actually are.

Robert Spitzenpfeil <madworm>
Mon 18 Jul 2011 06:32:55 PM UTC, comment #19: 

Hmmm. I have the strong urge to slap someone over the head with a stinking wet fish. But probably they are already dead... this is like hunting a misguided minus-sign on pages after pages of formulae.

As I don't see how avrdude should easily find out if it talks to an old-fashioned serial port or a 5V variant, it doesn't make much sense to talk about setting a signal line to a certain voltage level. Logic levels are all that remain accessible.

This brings me back to 'arduino.c'.

As 'arduino_open()' talks about charging/discharging capacitors and HIGH/LOW levels (and when I hear HIGH, I think about 5V - not 0), there should be clear additional comments about what writing a 'logic 0' or 'logic 1' to DTR actually do on the piece of wire in terms of a real measurable Voltage (not logic states).

Robert Spitzenpfeil <madworm>
Mon 18 Jul 2011 04:57:13 PM UTC, comment #18: 

Those are real full-swing (e.g. +-12V) RS-232 voltages.  The voltage levels are inverted for TTL (0 to 5V) serial, so the data signals are 5V for a logic 1 and 0V for a logic 0 (whereas -12V is a logic 1 for the data signals of full-swing serial) and the control signals (apparently) become 0V for logic 1 and 5V for logic 0.

David A. Mellis <mellis>
Mon 18 Jul 2011 03:43:28 PM UTC, comment #17: 

I have abandoned windows almost completely.

The only way I can run it is inside VirtualBox, and I don't think it is wise to use that as reference.

Identical behaviour across platforms is a very good thing!

According to wikipedia, the RS-232 control signals are not inverted.

[...]

When one of these signals is active [DTR...], the voltage on the line will be between +3 to +15 volts. The inactive state for these signals is the opposite voltage condition, between −3 and −15 volts. Examples of control lines include request to send (RTS), clear to send (CTS), data terminal ready (DTR), and data set ready (DSR).

[...]

So I would think that translating that to 0...5V would be just scaling and shifting the voltage interval without inversion.

I would assume that ser_set_dtr_rts(1) would therefore set the line HIGH (active), and ser_set_dtr_rts(0) to LOW.

Why the low-level register writes are inverted on linux I don't know. I'm trying to find out why that is the case. Finding the right kernel mailing list is a tedium.

Robert Spitzenpfeil <madworm>
Mon 18 Jul 2011 02:59:16 PM UTC, comment #16: 

Have you checked the behavior on Windows?  There, I believe that a call to ser_set_dtr_rts(1) brings the lines low and 0 brings the line high.  So patch #7100 brought the two OS's (Windows and Posix) into alignment: i.e. interpreting a 1 to mean "set the corresponding bit in the control register", which, on both operating systems, sets the physical line low.  But someone should probably verify this, since it's been a while since I tested it and my notes are a bit obscure. 

If I'm right, then I believe the current behavior (in SVN) is correct: it's consistent between operating systems, uses the OS interpretation of the meaning of "set the line" (i.e. bringing it low), and correctly resets Arduino boards (which is, after all, the goal). 

Maybe we just need to clarify with better comments the behavior of the ser_set_dtr_rts() function (i.e. that a logical 1 means a low level on the physical line and vice-versa)?

David A. Mellis <mellis>
Sun 17 Jul 2011 08:51:12 PM UTC, comment #15: 

Forget about what I've said about the ioctl(). The logic in there is reversed as well.

Nevertheless, I doubt that my logic analyzer lies to me. Clear the TIOCM_DTR bit and the line goes HIGH.

Robert Spitzenpfeil <madworm>
Sun 17 Jul 2011 08:45:00 PM UTC, comment #14: 

Hmmm. Not on my system.

From my point of view it looks like that patch #7100 is just wrong. It makes the auto-reset functional, but that's about it.

When I use that patch and have a look at DTR with my logic analyzer, the signal is inverted. (FTDI adapter).

I've attached some code that you can play with. Even the ioctl() that reads back the status of DTR shows that it is set LOW after having written a '1' to the bit, and set HIGH after having written a '0' to the bit.

Maybe the bug is somewhere deeper in the food chain.

I think that when you call ser_set_dtr_rts(1) the DTR line should actually be set HIGH. With your patch that is not the case on my machine.

(file #23669)

Robert Spitzenpfeil <madworm>
Sun 17 Jul 2011 06:35:12 PM UTC, comment #13: 

Have you tried the latest avrdude from SVN?

This looks like a duplicate of patch #7100 (which is already applied in the repository): http://savannah.nongnu.org/patch/index.php?7100

David A. Mellis <mellis>
Sat 16 Jul 2011 11:22:16 PM UTC, comment #12: 

"I do not know. There are several things I can't adequately explain with this board yet."

Looks like the supplier just reported a minor hardware defect.  I'll try to make the mod next weekend and let you know if it works better then.

Bernard Johnson <bjohnson>
Sat 16 Jul 2011 11:12:54 PM UTC, comment #11: 

"I don't want to turn this into a "which patch is better" discussion"

Nope, not that at all.  I'm just trying to share what I see and what if any success I'm having.

Thanks for verifying with a logic analyzer.

"Nevertheless nothing of this changes the fact that only a HIGH-->LOW transition can cause a reset pulse if there is a capacitor between DTR and the reset pin."

Agreed

"Toggling the line twice always creates at least one H->L transition."

Agreed, and my only concern was really around unintended consequences.  I can't really think of any though.

"Why does your arduino require an additional reset after the bootloader is done? So far every bootloader I've used automatically starts the application after the upload is done. No additional external reset required here."

I do not know.  There are several things I can't adequately explain with this board yet.

"Regarding holding DTR low. This only keeps the AVR in reset state if DTR is wired directly to the reset pin."

Sorry, what I meant was "as if held LOW" - The reason I say so is I can't even reset it with the reset button at this point.  It's literally like someone tied RESET to ground.


Bernard Johnson <bjohnson>
Sat 16 Jul 2011 10:25:33 PM UTC, comment #10: 

Only the comments in 'ser_posix.c' are quite misleading. If clearing a bit in a register actually sets the signal line to high and vice versa. Almost like clearing an interrupt flag in an AVR by writing a 1 to the bit.

Someone else on a linux system should check this as well. Just to rule out that my system is weird.

Nevertheless nothing of this changes the fact that only a HIGH-->LOW transition can cause a reset pulse if there is a capacitor between DTR and the reset pin. All arduinos have that.

Toggling the line twice always creates at least one H->L transition.

Robert Spitzenpfeil <madworm>
Sat 16 Jul 2011 10:15:03 PM UTC, comment #9: 

OK. On linux + FTDI chip, these are the facts. Measured with my logic analyzer on the DTR line.


/* set line to LOW */

ctl |= TIOCM_DTR;
ioctl(fdp->ifd, TIOCMSET, &ctl);

/* set line to HIGH */

ctl &= ~TIOCM_DTR;
ioctl(fdp->ifd, TIOCMSET, &ctl);


So it seems the code in 'ser_posix.c' is currently correct for the OS/hardware/driver constellation mentioned above.

Robert Spitzenpfeil <madworm>
Sat 16 Jul 2011 09:12:28 PM UTC, comment #8: 

Hmmm.

I don't want to turn this into a "which patch is better" discussion ;-)

The patch(es) that provide the best (and identical) results on all platforms should be used. Preferably with code that has the same logic and enough comments to understand it, if there are significant differences.

Now back to the reset business.

Why does your arduino require an additional reset after the bootloader is done? So far every bootloader I've used automatically starts the application after the upload is done. No additional external reset required here.

Regarding holding DTR low. This only keeps the AVR in reset state if DTR is wired directly to the reset pin.

To my knowledge all 'arduino'-type boards have a 100nF capacitor between the DTR line and the chip's reset pin. Therefore it is physically impossible that keeping DTR low holds the chip in reset. DC voltages are blocked by capacitors. That's why I keep repeating the HIGH-->LOW business all the time.

Anyhow. If getting this to work needs a 2nd reset on windows, then it should be inserted.

I will look into the ser_posix.c stuff some more and record traces.

Robert Spitzenpfeil <madworm>
Sat 16 Jul 2011 08:36:17 PM UTC, comment #7: 

I said serial_close below but I meant arduino_close.

Bernard Johnson <bjohnson>
Sat 16 Jul 2011 08:35:44 PM UTC, comment #6: 

Actually, if you look at my patch and then look at ser_win32.c, my patch makes the logic work the same on both platforms.  Windows has always been bulletproof (and I can't tell you how much it bothers me that it wasn't on Linux).  Once I made that change, it became just as reliable under Linux for me (Uno).

Now that being said, what I haven't mentioned is that is not the problem I was trying to fix.  I have a board with a FT232 and basically nothing is working under Linux (works just fine in Windows).  My patch at least gets the board reset after attempted programming, which was a step forward. 

With your patches, the close doesn't reset the device and start the code.  I think unless you intend to remove serial_set_dtr_rts function, the logic has to be reversed as I did otherwise places like serial_close will still be reversed.

You're right though, the code is just confusing.  For example is_on is unfortunately named because it implies current state rather than desired state.

Btw, multiple times now you have said that your device resets on a DTR transition to LOW: "Reset pulses can only occur at a HIGH-LOW transition".  Unless I misunderstand you (and I do not have a logic analyzer), my Uno goes into reset when DTR is pulled low, and then boots once DTR is released (verifed by LED).  So long as I have DTR/reset pulled low, it does not boot.  Is that what you see with your device?

So here is the end result with your patch:

Uno - works fine
Zigduino/FT232 - neither programs (this was also problem with my patch) or boots when closing the device (appears that reset is held LOW)

Bernard Johnson <bjohnson>
Sat 16 Jul 2011 05:48:16 PM UTC, comment #5: 

Info:

The latest proposed changes, provided in file #23663, assume that the patch given by file #23541 has already been applied.

Robert Spitzenpfeil <madworm>
Sat 16 Jul 2011 03:34:21 PM UTC, comment #4: 

Another option would be to introduce

serial_toggle_dtr_rts();

and in 'arduino.c' use:

  serial_toggle_dtr_rts(&pgm->fd);
  usleep(50*1000);

  serial_toggle_dtr_rts(&pgm->fd);
  usleep(50*1000);

That way it would not matter (for the purpose of auto-reset) if the stuff in 'ser_posix.c' is inverted or not - as long as it is consistent! Also the initial state of DTR is irrelevant.

It would either produce HIGH_LOW_HIGH or LOW_HIGH_LOW, both sequences reset the chip.

I've attached the changes needed for this proposal. But it adds more changes.

The generated waveforms on DTR and reset are the same as with my initial patch (trace #2)

(file #23663)

Robert Spitzenpfeil <madworm>
Sat 16 Jul 2011 12:28:59 PM UTC, comment #3: 

In the piece of code you've referred to it seems to be 'inverse logic'. That is if it gets a '1' it clears some bits. Yes.

As far as what happens on the physical DTR signal of my serial port: no. At least on FTDI chip based adapters.

I've made a few traces with my logic analyzer.

1) unpatched avrdude 5.10
2) my patch
3) your patch
4) both patches

It shows that the unaltered 'ser_posix.c' code actually drives the physical signal LOW, when the ser_set_dtr_rts() function gets a '0' as argument.

I think that the code should represent what happens in the real world and not in some register.

Further description of the attached image:

For each test run the arduino board was disconnected and reconnected to USB. This makes sure the serial port is in the same state.

1)

Avrdude is run for the first time and everything works. The AVR chips actually gets 2 reset pulses, which doesn't seem to matter.

Then avrdude is run 3 times in a row. Each run times out, as it can't get an answer from the bootloader. As you can see in the trace, the reset pulse is created very late. This is caused by the fact that the DTR line is not switched in the right way. Reset pulses can only occur at a HIGH-LOW transition.

The next time avrdude is run and interrupted with CTRL+C. This leaves the DTR line high.

The next and last run of avrdude works again, just like the very first time.

2)

With my proposed patch it works and the DTR line is always left in a usable state for next runs of avrdude. Actually it is independent of the initial state of DTR.


3)

With your patch (inversed DTR register writes), the upload works every time as well, BUT ser_set_dtr_rts() now does the inverse thing of what it is told to do. If it gets a '0' it writes a '1' to DTR and vice versa. I don't like that, as this is confusing. Maybe there should be a comment in 'ser_posix.c' why the register writes are inversed to produce the desired output on the physical signal line.

4)

With both patches applied it still works, but it is basically as confusing as 3).


Robert Spitzenpfeil <madworm>
Sat 16 Jul 2011 09:36:03 AM UTC, comment #2: 

Actually, I think you got better results by accident.  Isn't the logic in ser_posix.c ser_set_dtr_rts backwards?

Can you try the attached patch instead of your patch?

This fixed the problem with my Uno needing a manual reset to upload code under Linux.

(file #23660)

Bernard Johnson <bjohnson>
Mon 20 Jun 2011 06:24:55 PM UTC, comment #1: 

I have tested this with some of my projects, which are loaded with the 'optiboot' bootloader. So far it works 100%.

If if matters, I use an FTDI based USB/TTL-serial adapter.

Robert Spitzenpfeil <madworm>
Mon 20 Jun 2011 06:19:17 PM UTC, original submission:  

It came to my attention that I could only upload every 2nd try to my bootloaded AVR chip (or 'Arduino' if you like). The first pass would fail and I would abort avrdude (the chip was not reset). With the old code this left the DTR line in a usable state for the next run, which then succeeded reproducibly.

This patch reverses the sequence of setting/clearing DTR/RTS. In depth explanation including a small schematic are in the comment in the .diff file.

Robert Spitzenpfeil <madworm>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #23669:  test_dtr.c added by madworm (782B - text/x-csrc)
file #23662:  g3215.png added by madworm (157KiB - image/png)
file #23660:  rev-logic.patch added by bjohnson (545B - text/x-patch)
file #23541:  arduino.c.patch added by madworm (2KiB - text/x-patch - Arduino 'auto-reset' fix.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by joerg_wunsch (Posted a comment)
  • -email is unavailable- added by mellis (Posted a comment)
  • -email is unavailable- added by bjohnson (Updated the item)
  • -email is unavailable- added by madworm (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-08-24 joerg_wunsch Open/ClosedClosed Open
    2011-08-24 joerg_wunsch StatusNeed Info Wont Do
        Assigned toNone joerg_wunsch
        Open/ClosedOpen Closed
    2011-08-24 joerg_wunsch StatusNone Need Info
    2011-07-17 madworm Attached File- Added test_dtr.c, #23669
    2011-07-16 madworm Attached File- Added 0003-Added-serial_toggle_dtr_rts__just_testing___.patch, #23663
    2011-07-16 madworm Attached File- Added g3215.png, #23662
    2011-07-16 bjohnson Attached File- Added rev-logic.patch, #23660
    2011-06-20 madworm Attached File- Added arduino.c.patch, #23541

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code