patchAVR Downloader/UploaDEr - Patches: patch #9573, patch to add atmelice avr32...


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

patch #9573: patch to add atmelice avr32 programming support to avrdude

Submitted by:  Bradley Jarvis <bradley_jarvis>
Submitted on:  Thu 22 Feb 2018 10:16:28 AM UTC  
Category:  None Priority:  5 - Normal
Status:  Postponed Privacy:  Public
Assigned to:  Joerg Wunsch <joerg_wunsch> Open/Closed:  Open

Mon 21 Sep 2020 02:09:36 AM UTC, comment #3: 

No problem, I will update the patch following the formatting guides mentioned.

The function jtag3_edbg_recv_frame was only being called from jtag3_recv_frame and it had to be fixed so it was integrated into this function, from memory it was how it was handling fragmented frames. I initially implemented this patch on avrdude 6.1 back in 2015 and I believe in that version the jtag3.c module could not receive fragmented frames(besides I think my version is much clearer/cleaner ;).

I think the ISP comment is a copy/paste error, it should have been JTAG, thanks for picking that up.

I have made the changes and once I get a chance to test it on some hardware and then I will update the patch.

Bradley Jarvis <bradley_jarvis>
Sun 20 Sep 2020 08:43:22 PM UTC, comment #2: 

Thanks for submitting the patch!

First thing that springs to mind after applying it:

gmake[2]: Entering directory '/home/joerg/src/avrdude'
  CC       libavrdude_a-jtag3.o
jtag3.c:1031:12: warning: 'jtag3_edbg_recv_frame' defined but not used [-Wunused-function]
static int jtag3_edbg_recv_frame(PROGRAMMER * pgm, unsigned char **msg) {

So why is this function no longer going to be used?

After some closer inspection, I have a number of stylistic issues with the patch before I'd like to apply it.

+  id    = "atmelice_avr32";
+  desc  = "Atmel-ICE (ARM/AVR) in ISP mode";

Really in ISP mode?

+        if (data[0]==SCOPE_AVR32)
+        {
+            char *str=0;
+              case AVR32_FAILURE_RECEIVE_LENGTH: str="Incorrect packet length"; break;
+  if (len > (((USBDEV_MAX_XFER_3-4)*16)-4))

I would appreciate if the patch keeps the existing style rule of having white space around operators (=, == etc.).

-         avrdude_message(MSG_INFO, "\\%03o", data[i]);
+          //avrdude_message(MSG_INFO, "\\%03o", data[i]);
+          avrdude_message(MSG_INFO, "\\%02x", data[i]);
+#if 0
   /* 4 bytes overhead for CMD, fragment #, and length info */
   int max_xfer = pgm->fd.usb.max_xfer;
   int nfragments = (len + max_xfer - 1) / max_xfer;
@@ -507,6 +647,7 @@
       data += this_len;
       len -= this_len;

Source code comments and conditional compilation is not the way to keep track of history. This is what SVN (or whatever VCS is used) is for.

Also, as you can see in the first snippet, make sure to not artificially alter the indentation style. Unfortunately, AVRDUDE's code base was never very consistent on whether to use TAB or spaces, but if you change a single existing line, do not convert a TAB to spaces or vice versa.

+  //if ((buf = malloc(USBDEV_MAX_XFER_3)) == NULL) {
+  //  avrdude_message(MSG_INFO, "%s: jtag3_edbg_recv(): out of memory\n",
+       //    progname);
+  //  return -1;
+  //}

See above. If some piece of code isn't needed anymore, remove it, rather than commenting it out.

     if ((c == RSP3_FAILED) && ((*resp)[3] == RSP3_FAIL_OCD_LOCKED)) {
-                     "%s: Device is locked! Chip erase required to unlock.\n",
-                     progname);
+          "%s: Device is locked! Chip erase required to unlock.\n",
+          progname);
     } else {
       avrdude_message(MSG_INFO, "%s: bad response to %s command: 0x%02x\n",
-                     progname, descr, c);
+          progname, descr, c);

Again, random indentation changes, please avoid this. Tell you editor to not touch anything else but what you're actually editing.

-  free(resp);
+  if (resp) free(resp);

Unneeded. The C standard guarantees free(0) does nothing.

The entire patch looks quite intrusive to the existing jtag3.c file. Thus, it imposes quite a bit of risk of breaking things that are unrelated to the AVR32 addition (see the first warning about a now unused function).

For that reason, I'd like to defer inclusion of the patch until after the next AVRDUDE release. Sorry, I don't have the time and energy of maintaining a separate -stable branch.

However, even though I'm setting it to "postponed" now, please feel encouraged to re-submit the patch with the above things fixed.

Joerg Wunsch <joerg_wunsch>
Project AdministratorIn charge of this item.
Thu 22 Feb 2018 10:26:54 AM UTC, comment #1: 

This patch is in relation to bug #43178, I have had a few people ask me for the patch lately so want to add it back into avrdude. I just re-applied my patch made on 6.1 which was the original code base I worked with to the latest svn trunk version.

Bradley Jarvis <bradley_jarvis>
Thu 22 Feb 2018 10:16:28 AM UTC, original submission:  

This patch adds support for programming avr32 devices via the atmelice jtag programmer using the jtag3 driver. The update add a new programmer atmelice_avr32 which is used to access the new feature

Bradley Jarvis <bradley_jarvis>


Attached Files
file #43383:  avr32-jtag3.patch added by bradley_jarvis (74KiB - text/x-patch - patch avrdudeavrdude)


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 bradley_jarvis (Submitted the item)

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.




    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-09-20 joerg_wunsch StatusNone => Postponed
        Assigned toNone => joerg_wunsch
    2018-02-22 bradley_jarvis Attached File- => Added avr32-jtag3.patch, #43383

    Back to the top

    Powered by Savane 3.6