Improve ArduinoISP sketch #3500

Merged
merged 25 commits into from Oct 7, 2015

Projects

None yet

8 participants

@PeterVH
PeterVH commented Jul 10, 2015

This is the pull request for issue 3321

More info on this change request is here.

I will update the progress of testing effort in the table below:

Test report for linux (Kubuntu 14.04 LTS):

Test Duemilanove UNO Leonardo Due Mega
No target OK OK OK OK OK
Burn UNO bootloader todo todo todo todo todo
Burn Arduino Mega bootloader OK OK OK todo OK
Program an attiny85 (Chaucer4K) todo OK OK OK OK
Program an attiny2313 (TinyBlink) OK OK OK OK OK
Program an attiny841 (TinyBlink) todo OK OK OK OK
Program an at89s52 OK OK OK FAIL(1) OK
Chaucer112K to 1284p OK OK todo OK OK
Chaucer112K to Mega todo OK todo todo todo
LongStoryShort todo todo todo todo todo

(1) an at89s52 works out of spec at 3V3. Downloading hex files, signature etc works but flashing a new hex file requires at least 4V. With a shield containing a 5V buffer this could work...

Test report for windows:

Test Duemilanove UNO Leonardo(3) Due(3) Mega
No target OK OK(4) OK OK OK(4)
Burn UNO bootloader OK OK OK OK OK
Burn Arduino Mega bootloader OK OK OK OK todo (5)
Program an attiny85 (Chaucer4K) OK OK OK OK OK
Program an attiny2313 OK OK OK OK OK
Program an attiny841 OK OK OK OK OK
Program an at89s52 OK OK OK(1) FAIL(2) OK
Chaucer112K to 1284p todo todo OK OK todo
Chaucer112K to Mega todo OK OK OK todo (5)

(1) used pre compiled hex files, available in the ArduinoISP-test repo.
(2) an at89s52 works out of spec at 3V3. Downloading hex files, signature etc works but flashing a new hex file requires at least 4V. With a shield containing a 5V buffer this could work...
(3) requires the changes to CDC.cpp proposed in #1182.
(4) the error led flashes briefly because the 16u2 sends a bogus byte when changing the baud rate, this is harmless, stk500v1 recovers from it
(5) only big spenders have two mega's

@ffissore
Contributor

It doesn't merge. Can you please rebase the code on top of latest master?

@cmaglie cmaglie was assigned by ffissore Jul 10, 2015
@facchinm
Member

Thank you @PeterVH ! Unfortunately a recent change moved the sketch from ArduinoISP folder to 11.ArduinoISP/ArduinoISP to be coherent with the rest of the examples.

Could you rebase upon the current master branch?
Thank you very much

@facchinm facchinm assigned facchinm and unassigned cmaglie Jul 10, 2015
@facchinm facchinm added the ArduinoISP label Jul 10, 2015
@facchinm facchinm changed the title from Issue 3321 to Improve ArduinoISP sketch Jul 10, 2015
Randall Bohn... and others added some commits Jan 31, 2012
Randall Bohn (Huckle) Be more specific about datatypes in the parameter struct. a2a06f5
Randall Bohn (Huckle) Don't start_pmode if we're already in pmode. d968c7a
Randall Bohn (Huckle) Set error if GET_SIGN_ON not followed by CRC_EOP 8dfe833
Randall Bohn (Huckle) Use SPI library. aeaed27
Peter Van Hoyweghen Avoid delay in heartbeat. That way we can also set the baudrate back …
…to the value used in the IDE. In fact, with this fix, baud rates of up to 115200 work also.
ad2a32f
Peter Van Hoyweghen Simply use pin 10 to reset the target. It works for all Arduino's and…
… allows the same "ArduinoISP shield" to be used on all of them. Such a shield has an ISP connector that: - wires MISO,MOSI, SCK, GND and 5V to the ISP connector of the programming Arduino - wires RESET to pin 10 of the programming Arduino.
93b31b6
Peter Van Hoyweghen The delay between reset and the enter progmode command got lost when …
…introducing the SPI library. Put it back in.
56e0910
Peter Van Hoyweghen Support at89sx mcu's by adding the possibility to specify the polarit…
…y of the target reset signal.
15da182
Peter Van Hoyweghen Configure SPI pins as input when target is not in reset. a17a757
Peter Van Hoyweghen Implement bitbang SPI for slow clock devices. (Sylvan Butler). 64e52d8
Peter Van Hoyweghen Configure the serial port to use. Prefer native USB port if the Ardui…
…no has one.
c8b5e99
Peter Van Hoyweghen Fix typo: SPI_CLOCK_DIV128 eca360b
Peter Van Hoyweghen Don't pulse error led if avrdude enters programming mode twice 5a04ab2
@PeterVH
PeterVH commented Jul 13, 2015

@facchinm: Done. But this forced me to push -f again.

@facchinm
Member

@PeterVH no problem at all, thank you very much! I'm launching the PR builder to get a complete IDE image with your sketch so anyone interested can test it!

Peter Van Ho... added some commits Jul 14, 2015
Peter Van Hoyweghen Ensure minimum spi pulse width. 50f9e53
Peter Van Hoyweghen Use new style SPI::beginTransaction() api, make SPI_CLOCK configurabl…
…e, select hardware or software SPI based on SPI_CLOCK.
89184a3
@q2dg
q2dg commented Jul 18, 2015

@PeterVH Maybe it's a sillyness, but do you know https://github.com/adafruit/ArduinoISP ?? For gathering some ideas...

@PeterVH
PeterVH commented Jul 23, 2015

I am afraid I introduced some regression. Some tests, e.g. the at89s52 fail (burning works but verifying fails...) Will have to check this out.
B.t.w. there is partial information on the tests here

Peter Van Ho... added some commits Jul 28, 2015
Peter Van Hoyweghen Call SPI.beginTransaction() after SPI.begin() 8d95899
Peter Van Hoyweghen BitBangedSPI::begin(): initialize levels of SCK and MOSI. Correct ind…
…entation.
d271c1c
Peter Van Hoyweghen Fix warnings. Use unsigned int to represent a (word) address: the com…
…piler will use the most efficient type on each platform: 32 bit on arm,

16 bit on avr which is is big enough.
044a7b7
@PeterVH
PeterVH commented Jul 29, 2015

It is fixed, several things were wrong: see commit messages.

Also, omitting the error led pulses on duplicate enter pmodes (commit 5a04ab2) had a side effect for the at89s52: these pulses took 180 msec and the at89 needed this as an extra delay after the chip erase. The correct way to deal with this is through the chip_erase_delay attribute in avrdude.conf. The at89s52 datasheet says this takes 'about 500 msec' so I use 600000. Now my knight rider hex files burn happily again.

Finally I fixed the compiler warnings.

@PeterVH
PeterVH commented Jul 29, 2015

I think this is about it for now. I will start more systematic testing now. I will update the progress of this in the first entry of this issue.

If someone has access to a Zero or even a Gallileo (@facchinm ?) and could give it a try (even a subset of the tests) that would be great. Also I have no mac. Of coarse every other form of help in testing is welcome (a Windows adept certainly would save me pain).

@q2dg : it is good to take a look at other forks indeed, and I do that regularly. But note that my approach is to focus on fixing up the sample delivered with the IDE, rather than adding new
features (the at89s52 support being an exception).

@facchinm
Member

@ArduinoBot build this please

@facchinm
Member

Thank you very much @PeterVH for your effort! I'll test it soon on more board and on every OS! 🤘

@facchinm
Member

Hi Peter, I and @cmaglie extensively tested the sketch on a great variety of boards (not with a lot of targets unfortunately).
Zero is working after applying this small patch

digitalWrite(MOSI, (b & 0x80) ? HIGH : LOW);

instead than

digitalWrite(MOSI, b & 0x80);

Other platforms using bitbanging works well (Galileo, Edison, Tre and obviously Due).
We didn't test at89s52 or attiny targets but I'm quite confident they should work if working on Due (which you can use as reference platform for bitbanging).

I'd merge it after patching for Zero; in the meantime if you can share the test suite we can setup some automatic testing environment with all the other boards.

Thank you very much!

@matthijskooijman
Contributor

@facchinm, isn't this a limitation of the Zero core? Even though digitalWrite is documented to accept HIGH and LOW, in practice it accepts any zero or non-zero value. Is there a compelling reason for the zero not to?

@facchinm
Member

@matthijskooijman you are right, the Zero's digitalWrite() is a bit too strict.
If we change

  switch ( ulVal )
  {
    case LOW:
      PORT->Group[g_APinDescription[ulPin].ulPort].OUTCLR.reg = (1ul << g_APinDescription[ulPin].ulPin) ;
    break ;

    case HIGH:
      PORT->Group[g_APinDescription[ulPin].ulPort].OUTSET.reg = (1ul << g_APinDescription[ulPin].ulPin) ;
    break ;

    default:
    break ;
  }

into

  switch ( ulVal )
  {
    case LOW:
      PORT->Group[g_APinDescription[ulPin].ulPort].OUTCLR.reg = (1ul << g_APinDescription[ulPin].ulPin) ;
    break ;

    default:
      PORT->Group[g_APinDescription[ulPin].ulPort].OUTSET.reg = (1ul << g_APinDescription[ulPin].ulPin) ;
    break ;
  }

it works out of the box. Can you see any drawback in this change? I'm opening a PR on Zero core repo anyway 😄

@facchinm
Member
@NicoHood
Contributor

Also see this:
#3687

And this (not completely related):
#3568 (comment)

I did a quick test with HoodLoader2 and it seems to work (burned a 32u4 HL2.0.5 Bootloader). It would be nice if you could add pin4 for HoodLoader2 devices, since they normally dont have pin 10 broken out and u2 devices are now supported by the IDE.

Here are my changes (except style changes) that I used before to use the ISP sketch with all kind of Arduinos. Maybe this helps:
https://gist.github.com/NicoHood/8f074c34fcc9a2a0a627/revisions

@PeterVH
PeterVH commented Aug 25, 2015

Hi all,

Thanks for the feedback and especially for the testing effort. Great to hear it works on all these boards. I have been on holiday without network access which is why it was silent from my side.

@facchinm:

  • about the test suite: it just consists of the description (still rather incomplete) of the various use cases and the source code of the test sketches I use, these are here.
  • will do the digitalWrite(HIGH/LOW ) fix in the sketch also.
  • Unfortunately I overlooked something important: I wanted to absolutely make sure the target is in reset before MOSI or SCK are driven in order to prevent driver contention. This is important especially on these more vulnarable 3V3 boards like the Due... See start_pmode(): right now this is not guaranteed. I had such code in my due branch from 2 years ago. I integrated this code in this PR. It works but from time to time the error led blinks. I still need to investigate this.

@NicoHood: HoodLoader looks cool, will configure pin 4 if ARDUINO_HOODLOADER2 is defined.
About #3687 (or my #1182): We must indeed fix this or things still don't work out of the box on Leonardo. Still not sure what the best solution is. Will comment over there.

Peter Van Hoyweghen Reset target before driving SCK or MOSI, reset sequence as in AVR dat…
…asheets under "Serial Programming Algorithm"
13cc57e
@PeterVH
PeterVH commented Aug 26, 2015

I have found the problem, so I pushed the code. There was nothing wrong with the new reset sequence code. I was testing with a Mega as programmer and the serial port was set to a baud rate different from 19200, so avrdude has to set the correct baud rate on the fly. This baud rate change causes the Mega to receive a garbage 0xff byte. The stk500 protocol however recovers from this. Interestingly a duemilanove does not have this problem (ftdi usb to serial converter).

I ran most of the tests the tests again, it looks stable.

So 2 micro things left to do on the sketch: hoodloader config and zero thing.

@NicoHood
Contributor

@PeterVH the mega and the uno have a 16u2 as USB-Serial bridge.

I have two questions regarding this:

  • Does the Uno behave the same with the garbage input?
  • Could you test the same settup with HoodLoader2 (not as ISP, as bridge only). Because I might have fixed this garbage byte in my bootloader. If not maybe we can correct this error (at least for hoodloader2 since many people start using it)

Also I do not really understand what you mean with

so avrdude has to set the correct baud rate on the fly.

Does the ISP now work with the Micro/Leonardo?

@PeterVH
PeterVH commented Aug 27, 2015

Does the Uno behave the same with the garbage input?

Yes. I just tried it out.

Also I do not really understand what you mean with:
so avrdude has to set the correct baud rate on the fly.

Well, avrdude sets the baudrate when it starts programming, this causes a baud rate change in the u2 and this causes the Uno or Mega receive a garbage byte.

Could you test the same setup with HoodLoader2

Sure. Will first read about hood loader. I intend to use it as a more convenient way to disable autoreset (instead of the capa) on Mega and Uno. (Currently on the Mega and Due I use a modified u2 firmware that disables autoreset if a jumper is placed over a pair of the 4 broken out gpios)

Does the ISP now work with the Micro/Leonardo?

Yes. See the test report I maintain at the top of this thread. (Be it that on windows you still have to do -carduino).

Peter Van Ho... added some commits Aug 30, 2015
@PeterVH
PeterVH commented Aug 30, 2015

@NicoHood: I can confirm that the problem of the garbage byte does not occur when using HoodLoader2 as bridge.
I added pin configuration for hoodloader2 and tested it.

B.t.w, it is a nice concept you worked out. I plan to keep it installed on my Uno. Though for my use case I'd like the inverse the logic: reset twice => sketch mode, press once or powerup => bootloader mode. That way I could leave ArduinoISP in the u2 and when I want to program something, I would activate the sketch by resetting twice. I'll have a look in the code whether that is easy to do...

@NicoHood
Contributor

So its a problem of the firmware, good to know.

This line fixes this (i think)
https://github.com/NicoHood/HoodLoader2/blob/dev/avr/bootloaders/HoodLoader2/HoodLoader2.c#L852
https://github.com/NicoHood/HoodLoader2/blob/dev/avr/bootloaders/HoodLoader2/HoodLoader2.c#L921

I could invert the logic, but by default it doesnt make sense to me.
What you can do is:

So maybe its time to update the firmware? The Leonardo bootloader also has some things that can be improved. The Arduino team should think about updating the unos firmware/bootloader and the leonardos bootloader as well.

@PeterVH
PeterVH commented Sep 21, 2015

@facchinm: Hi Martino.I ran the bulk of the tests again, this time on Windows. Everything worked fine on this os as well, (see table at the top). Also in the forum, the sketch was able to solve various problems. (this one being the most enthousiastic one :-) ). So I am now confident this is ready to be merged.

On windows I still need the fixes from #1182 for leonardo and due. I should make a separate pull resuest for that.

@cmaglie: you reported success on the Zero. I bought an Arduino M0 (not Pro) to be able to run that kind of tests myself. (You know I don't have access to Zero from Europe). I run with the arduino.org bootloader and use of coarse the arduino.cc ide and core. I adapted the .cc linker script so that the FLASH addresses match what the bootloader expects. The sketch runs: heartbeat led fades in out. But the communication over SerialUSB does not work correctly (see here). I see no a-priory reason why the behavior of an M0 would be different than on a Zero (but I may overlook something essential, any ideas?). The symptoms look like problems in the USB driver. Are you sure the sketch (still) works correctly on the Zero?

@facchinm
Member

Hi Peter, I'm really happy to hear such good news!

The Zero issue is probably related with SerialUSB code being quite problematic, but it was solved in arduino/ArduinoCore-samd#27 ; that PR will be merged soon but you can test it via Board Manager before release 😉

Anyway, I'm going to test the actual code (you to make sure, I'm totally convinced it works great) and merge it soon.

@NicoHood
Contributor

You may want to try if this works for you:
#3839

@PeterVH
PeterVH commented Sep 22, 2015

@fachinm: you were right about the usb problems, with the pr you mentioned I can now happily test ArduinoISP on my M0. Thanks! (This also proves ArduinoISP is a good test case for new cores. In the past it already brought problems in the Leonardo and Due cores to the surface)

There is another detail I wanted to bring to your attention. Someone in the forum brought up that it is confusing having to upload a sketch called 'ArduinoISP' whilst having to specify 'Arduino as ISP' as a programmer. He suggested to rename the sketch to ArduinoAsISP. I don't know. It is true more and more people in the forum start to refer to this programming method as 'Arduino as ISP'. I'll leave this matter to the Arduino devs...

BTW. the info about ArduinoISP on the new examples web site is inaccurate and adds more confusion (with a link to the ArduinoISP board).

@NicoHood: that would work for me. Some comments:

  • Could you also modify the Due core? Will test it if needed.
  • Could you also adapt the comment, it does not hold anymore
  • The configured flag is not cleared upon disconnection, but that might not be that important right now.
@PeterVH PeterVH closed this Sep 26, 2015
@PeterVH PeterVH reopened this Sep 26, 2015
@NicoHood
Contributor

It is not cleared on disconnect? Where do we have to handle this? (this is very important)
I only found this:

https://github.com/NicoHood/Arduino/blob/patch-8/hardware/arduino/avr/cores/arduino/USBCore.cpp#L724

In the lufa code are several parts where this variable is changed:
https://github.com/abcminiuser/lufa/blob/master/LUFA/Drivers/USB/Core/AVR8/USBInterrupt_AVR8.c#L127

@PeterVH
PeterVH commented Sep 26, 2015

I have found my way to your pr #3839, will comment over there

@facchinm facchinm merged commit 651ae04 into arduino:master Oct 7, 2015
@facchinm
Member
facchinm commented Oct 7, 2015

Time for merge 😄 😄 Thanks a lot Peter!

@ffissore ffissore modified the milestone: Release 1.6.6 Oct 7, 2015
@PeterVH
PeterVH commented Oct 7, 2015

Thanks Martino!
Fyi, here is a recent testimony I liked from someone using the sketch to burn a bootloader to an atmega 2560, which failed with an usbasp. I would really enjoy people coming back to using ArduinoISP for burning bootloaders and tiny firmware...

@NicoHood
Contributor

The leonardo windows bug is still not solved, correct?
Just gave it a quick test with HoodLoader2, burned a uno bootloader. Ultra fast. Pin selection is also correct, good job Peter!

@q2dg
q2dg commented Oct 19, 2015

Why there is a "Programmer->ArduinoISP" menu option and a "Programmer->Arduino as ISP" whereas there is only one "Examples->ArduinoISP" example sketch?? Which programmer is the good one?? It's very confusing.

@PeterVH
PeterVH commented Oct 19, 2015

"Arduino as ISP"
see above : (#3500 (comment))

@q2dg
q2dg commented Oct 19, 2015

Thanks. I've seen "ArduinoISP" programmer refers to https://www.arduino.cc/en/Guide/ArduinoISP . In this case, I think this should be marked in menu as "retired" or something similar, but I'm not sure.
Sorry for bother you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment