Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wiring: set reset discharge time to 80ms #1504

Closed
wants to merge 1 commit into from
Closed

Conversation

diresi
Copy link

@diresi diresi commented Sep 26, 2023

Geeetech GT2560 boards [1] use 1uF capacitors in their DTR/RESET line and 50ms seem to be a tad too short. 80ms works well (here).

fixes #1503

[1]
https://www.geeetech.com/wiki/images/d/d3/Hardware_GT2560_RevA%2B.pdf

Geeetech GT2560 boards [1] use 1uF capacitors in their DTR/RESET line
and 50ms seem to be a tad too short. 80ms works well (here).

fixes avrdudes#1503

[1]
https://www.geeetech.com/wiki/images/d/d3/Hardware_GT2560_RevA%2B.pdf
@stefanrueger
Copy link
Collaborator

Great to know this fixes comms with your board, and thanks for the PR!

@dl8dtl @MCUdude @mcuee @mariusgreuel What do we do about other boards with a 2 µF, 10 µF, ... 1 mF cap between DTR and Reset?! OK, I realise there has to be a limit in what board design AVRDUDE can support (and to which waiting times we can subject applications to). Is it worth our while to introduce an -xdelay parameter or should we simply set the discharge time to 100 ms? If the former, there should be an error message when using -c wiring and running into synch problems that they should try -xdelay ... similarly to -c urclock (one issue with that is that stk500v2.c is already quite overloaded and I find it hard to predict which error messages @diresi would have seen).

@MCUdude
Copy link
Collaborator

MCUdude commented Sep 26, 2023

100nF is the norm, but some boards have apparently used 1uF, as this worked with older versions of Avrdude.

Is it worth our while to introduce an -xdelay

That would be a good idea. Perhaps name the flag so it's a little more self-explanatory, like -xrst_delay.

@dl8dtl
Copy link
Contributor

dl8dtl commented Sep 26, 2023

Since this is a per-board setting, how about introducing a config file parameter that can be adjusted per board definition?

@mcuee mcuee added enhancement New feature or request bug Something isn't working labels Sep 26, 2023
@diresi
Copy link
Author

diresi commented Sep 26, 2023

You guys are quick to respond :)

As I mentioned in #1503 I haven't thought this through at all and just posted this PR to show what works for me.
Please feel free to reject/update this PR as you wish!

To answer the question of @stefanrueger about errors I've seen, it was plain "timeout error"s right at the first recv in getsync().

For another thing, would you mind explaining or pointing me somewhere how that reset with serial cap stuff works. I can't quite understand it...

@stefanrueger
Copy link
Collaborator

config file parameter that can be adjusted per board

Technically, we only have programmer and part entries, so this would fall under programmer like so

programmer parent "wiring"
    id                  = "wiring-GT2560";
    add_t_rst_discharge = 30; # ms, can be negative         
;

Plus: keeps the command line clean (use -c wiring-GT2560 instead of -c wiring), generic method for many boards and personal preferences (I like my delays short and board response zippy); minus: proliferation of programmer entries and it's unclear how a user would figure out that this programmer parameter exists and how to use it.

@stefanrueger
Copy link
Collaborator

stefanrueger commented Sep 26, 2023

@diresi These reset circuits are not for the faint-hearted, and some hardware people I have spoken to roundly reject the plucking-a-cap method to create a reset pulse. Have a look at this simulation or get some popcorn and read the discussion in #1262 and #1309.

@diresi
Copy link
Author

diresi commented Sep 26, 2023

I thought about it in the past minutes and think I might get the idea, which also would explain the positive spike when setting DTS again (my board apparently doesn't have any diodes there and it shows).
Anyway, let me cross-check with your links :)

Thanks!

@stefanrueger
Copy link
Collaborator

plain "timeout error"s right at the first recv in getsync()

Thanks, so it would be this line?

pmsg_error("timeout\n");

The reason I ask is that it is all well and good if we have a mechanism to cater for various boards, but we should have a mechanism to tell a user who experiences problems what they might try to counter them. So, if we can give some hint in the right error message that would be useful.

@stefanrueger
Copy link
Collaborator

stefanrueger commented Sep 26, 2023

@diresi BTW, it will not be the cap alone that delays the board coming out of reset. Some fuse settings delay the MCU coming out of reset for a large start-up time (more than 65 ms) to give the oscillators time to settle down. For a list type

$ avrdude -qqc dryrun -p m2560 -T "config sut="

If SUT is large then AVRDUDE's first synch command won't be picked up by the MCU (still effectively in reset) and the expected response won't be coming forth. Hence, the timeout.

@dl8dtl Given that it's a combination of board hardware and SUT fuse setting, I now tend towards a -xdelay ... option.

@stefanrueger
Copy link
Collaborator

Thinking a bit more about this, I predict this PR won't help in case the cap C9 was bridged, ie, DTR directly connected to RESET. Originally, #1309 wanted to support programming boards with such a direct connection. For that one would need a (variable?) usleep(80*1000) after line 186

avrdude/src/wiring.c

Lines 182 to 186 in 8ed9615

serial_set_dtr_rts(&pgm->fd, 1);
usleep(50*1000);
/* Set high, so a direct connection to reset works. */
serial_set_dtr_rts(&pgm->fd, 0);

@mcuee @diresi Could you test that?

@mcuee you might need a large SUT fuse setting, eg, conf sut_cksel=extxosc_8mhz_xx_258ck_65ms (needs an external programmer and use of the terminal avrdude -t); @diresi won't need that, I predict his board's MCU is already on a large SUT setting.

@diresi
Copy link
Author

diresi commented Sep 26, 2023

@stefanrueger

Confirmed, it's exactly that timeout in line 717 of stk500v2.c.

However, moving usleep after line 186 like this doesn't work:

diff --git a/src/wiring.c b/src/wiring.c
index ae99b304..e0c9bbd7 100644
--- a/src/wiring.c
+++ b/src/wiring.c
@@ -184,6 +184,7 @@ static int wiring_open(PROGRAMMER *pgm, const char *port) {
 
     /* Set high, so a direct connection to reset works. */
     serial_set_dtr_rts(&pgm->fd, 0);
+    usleep(80*1000);
   }
 
   /* drain any extraneous input */

It needs to be the usleep in line 183:

diff --git a/src/wiring.c b/src/wiring.c
index ae99b304..ea937ab1 100644
--- a/src/wiring.c
+++ b/src/wiring.c
@@ -180,7 +180,7 @@ static int wiring_open(PROGRAMMER *pgm, const char *port) {
     pmsg_notice2("wiring_open(): asserting DTR/RTS\n");
 
     serial_set_dtr_rts(&pgm->fd, 1);
-    usleep(50*1000);
+    usleep(80*1000);
 
     /* Set high, so a direct connection to reset works. */
     serial_set_dtr_rts(&pgm->fd, 0);

I've played with timeouts and 75ms don't yet work but 80ms do, although both DTR and RESET look pretty similar on the scope, as you can probably imagine.
Using 75ms timeout:
gt2560-reset-75ms

And with 80ms timeout:
gt2560-reset-80ms

(the upper signal is DTR, the lower one is RESET).

@stefanrueger
Copy link
Collaborator

moving usleep after line 186 like this doesn't work

You still need a small usleep(20*1000) in line 183 (looking at your scope images reset is already Vcc at 20 ms, and any longer sleep won't change that RESET remains inactive). However, if you have only 20 ms in line 183, you will need at least usleep(60*1000) after line 186. I think your fuses are set so that you have a large SUT of 65+ ms.

Now, if you had had a direct DTR/RESET connection (bridge C9), then you'd need a bit more than usleep(60*1000) after line 186, otherwise this fails (that's my prediction, anyway).

@stefanrueger
Copy link
Collaborator

I don't know if the wiring bootloader reads the fuses (a bootloader defo cannot write them), but if it does, then you'd see your fuse settings via avrdude -qqc wiring -p m2560 -T config and could confirm a large SUT.

@diresi
Copy link
Author

diresi commented Sep 26, 2023

I still had the 50ms usleep in line 183 and also added 80ms in line 187, if that's what you mean.

The -T config shows this:

$ ./build_linux/src/avrdude -qqc wiring -p m2560 -T config -P /dev/ttyUSB0 
config sut_cksel=extxosc_8mhz_xx_16kck_65ms # 63
config ckout=co_disabled # 1
config ckdiv8=by_1 # 1
config bootrst=boot_section # 0
config bootsz=bs_4096w # 0
config eesave=ee_erased # 1
config wdton=wdt_programmable # 1
config spien=isp_enabled # 0
config jtagen=jtag_disabled # 1
config ocden=ocd_disabled # 1
config bodlevel=bod_2v7 # 5
config lb=no_lock # 3
config blb0=no_lock_in_app # 3
config blb1=no_lock_in_boot # 3

@stefanrueger
Copy link
Collaborator

stefanrueger commented Sep 26, 2023

Good, so assuming a 16 MHz external quartz the MCU's SUT is 65 ms + 1 ms (for 16k extra cycles). This means the MCU will jump to the bootloader 66 ms after the reset signal is no longer active, which is around 10-11 ms after line 182, ie, when DTR physically pulled low, which means setting it to 1 as it's an active low logic. So makes sense that you'd have to wait at least these 77 ms after line 182.

What I don't understand, is why it matters when DTR is released again. That, in my mind, could be anytime after the code has waited an initial 20 ms after line 182 (your scope vividly shows RESET is no longer active then). I cannot explain why keeping 50 ms in line 183 and adding 80 ms in line 187 does not work. Additional sleep in line 187 (within reason) should not matter. Or does it? Hmm, that might depend on the receive timeout setting. What happens if the code sleeps 30 ms in line 187 and keeps 50 ms in line 183?

@diresi
Copy link
Author

diresi commented Sep 26, 2023

I think this is what I tried earlier, isn't it? avrdude was sleeping for 50ms in line 183 and some additional 80ms after setting DTR high again in line 186. Do you think changing the latter to only 30ms would make a difference?

I just tried that again to make sure I didn't mess up earlier, but what can I say ... doesn't work :)

@stefanrueger
Copy link
Collaborator

stefanrueger commented Sep 26, 2023

changing the latter to only 30ms would make a difference?

In case the receive timeout is set to a short time, yes. Other than this, it should not matter if the code sleeps 80 ms after plucking DTR (initiating reset) and then releasing DTR or to sleep 50 ms, release DTR and then sleep 30 ms. If it does, I would not know why. Another idea: What's Vcc? If > 5.5 V then the > 11 V peak might trigger HV programming.

@Ho-Ro
Copy link
Contributor

Ho-Ro commented Sep 26, 2023

What's Vcc? If > 5.5 V then the > 11 V peak might trigger HV programming.

Good point, but isn't this risk an arduino HW issue and not an avrdude SW topic.

@stefanrueger
Copy link
Collaborator

stefanrueger commented Sep 26, 2023

arduino HW [vs] avrdude SW

Sure!

This PR wants to change a timing, which helps one specific board, and I'd like to understand what the actual cause for the underlying issue is and exactly what the ramifications for other boards are. Worst case for this PR is that it works for one board, but all hell breaks loose for a gazillion of other boards. @mcuee does a great job in testing some m2560 boards with wiring bootloader against regression, and I want to have some credible understanding of what happens...

Trouble is, I don't! I just checked that serial_recv_timeout is neither touched by stk500v2.c nor by wiring.c so each serial read should time out at its initial setting of 5 s throughout -c wiring. So, the error message should appear after 5 s in case @diresi's board fails programming. It should be OK to wait for up to 4.5 s in line 187. And I am at a loss why there needs to be a more than 20 ms wait in line 183 as the scope pictures demonstrate RESET is no longer active after 20 ms ... I do understand that there should be a 66 ms wait after release of reset.

@diresi
Copy link
Author

diresi commented Sep 26, 2023

Sorry for the delay, I can't play around all day, can I? :-)

I had a quick Vcc measurement and it really is 5.6V. It's attached to a RPi3 via USB but also has its own power supply, idk how that mixes together in detail.
Anyway, that supports the HV idea

I'll try adding surge(?) diodes later to confirm or not.

@diresi
Copy link
Author

diresi commented Sep 26, 2023

And yes, timeout errors are reported after several seconds each, 5s sounds pretty realistic.

@MCUdude
Copy link
Collaborator

MCUdude commented Sep 26, 2023

I'll try adding surge(?) diodes later to confirm or not.

I always use a BAT54S diode connected to VCC and GND, to prevent positive or negative voltage doubling. A design without a clamping diode of some sort is something I'd consider "unsafe", as you might accidentally trigger high-voltage programming mode, as pointed out earlier in this thread.

Here's a snippet of the USB to serial part of this ATtiny board I've designed (for ATtiny13/25/45/85), where VCC can be either 3.3 or 5V depending on the on-board target voltage switch.

image image

@stefanrueger
Copy link
Collaborator

surge(?) diodes

Any simple clamp diode will do, see recommended AVR reset circuit

@stefanrueger
Copy link
Collaborator

negative voltage doubling

Should be taken care of by the internal clamp diode

@stefanrueger
Copy link
Collaborator

5s sounds pretty realistic

That's not great in terms of user experience. stk500v2_getsync() should reduce at its start serial_recv_timeout to a small amount, so that retries make sense. There is only one point when a huge timeout in the order of 10 s makes sense: when AVRDUDE asks the chip be erased and the wiring bootloader emulates that by erasing all flash pages below the bootloader (I don't know whether it actually does that, optiboot simply ignores that),

@diresi
Copy link
Author

diresi commented Sep 26, 2023

I can now confirm that it is HV programming mode. Adding a random clamp diode (that I had lying around) makes RESET max out at some 6.5V or so and fixes the whole problem, vanilla avrdude 7.2 works well then.

I understand that this leaves open the whole discussion about what and how to fix but I trust that you'll take it from there (and I'm sure I'm way out of my league already).
As for the 5s-timeout-user-experience, as a hobbyist I don't mind that much. Sure, quick responses and exponential backoff are what people might be used to generally, but hey, this is an MCU programming tool. Rough edges are expected IMO.

Thanks everyone and especially @stefanrueger for the help so far, I had a fun day, learned quite something and fixed my problem all at the same time! And finally have some justifications for having bought a scope in the first place :)

@dl8dtl
Copy link
Contributor

dl8dtl commented Sep 26, 2023

Given that it's a combination of board hardware and SUT fuse setting, I now tend towards a -xdelay ... option.

Sounds good to me. Allows for a per-programmer default value that can be overridden on the commandline for pathological cases.

@dl8dtl
Copy link
Contributor

dl8dtl commented Sep 26, 2023

as you might accidentally trigger high-voltage programming mode, as pointed out earlier in this thread.

du-oh, I never thought about that :-/

@stefanrueger
Copy link
Collaborator

Given that it's a combination of board hardware and SUT fuse setting, I now tend towards a -xdelay ... option.

Sounds good to me.

Implemented in PR #1505

| I can now confirm that it is HV programming mode.

@diresi That's interesting to know. I believe Vcc = 5.6 V is out-of-specs for the ATmega2560, and putting a board briefly into HVPP through plucking the DTR/RTS lines is not something that AVRDUDE can do a lot about. I actually do not know why increasing the usleep() to 80 ms has helped, maybe hardened AVR experts can help out here? I am looking to @dl8dtl, @MCUdude and @mcuee to solve that riddle! I would have naively thought that radically decreasing that time to, say, 15 or 20 ms might have prevented the cap to fully charge to Vcc = 5.6 V, maybe only to 4 V, and that when releasing DTR/RTS back that then only 4 V would have been pushed onto Vcc for the Reset spike. While PR #1505 has chosen 20 ms this was not to help cope with an accidental HVPP reset, it was b/c theory and your scope pictures show that 20 ms is plenty, even for a large 1 µF cap.

stefanrueger added a commit to stefanrueger/avrdude that referenced this pull request Sep 27, 2023
Standard AVR reset circuits deploy a connection from the DTR/RTS line of a
USB-to-serial chip to the AVR via a small capacitor, see

  https://onlinedocs.microchip.com/pr/GUID-F626284A-58F0-4C25-A6F3-0EA5054F3E2B-en-US-6/index.html?GUID-B80B25FF-E9D7-4766-B562-DA197B8B938C

Setting DTR/RTS low once will issue a reset. Setting this signal high
again after a short time ensures that a direct connection DTR/RTS to reset
also works. The duration of pulling DTR/RTS low must be relatively short,
say RC/10, in order to avoid a reset spike above Vcc. If Vcc exceeds 5.5 V
then a full 2 Vcc reset spike can potentially trigger HV programming.

See also
 - avrdudes#1504 (comment)
 - avrdudes#1505 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7e94ed4442b breaks flashing GT2560 boards
6 participants