[patch #9573] patch to add atmelice avr32 programming support to avrdude

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch #9573] patch to add atmelice avr32 programming support to avrdude

Kevin Cuzner-2
URL:
  <http://savannah.nongnu.org/patch/?9573>

                 Summary: patch to add atmelice avr32 programming support to
avrdude
                 Project: AVR Downloader/UploaDEr
            Submitted by: bradley_jarvis
            Submitted on: Thu 22 Feb 2018 10:16:28 AM UTC
                Category: None
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any

    _______________________________________________________

Details:

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



    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Thu 22 Feb 2018 10:16:28 AM UTC  Name: avr32-jtag3.patch  Size: 74KiB  
By: bradley_jarvis
patch avrdudeavrdude
<http://savannah.nongnu.org/patch/download.php?file_id=43383>

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9573>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
avrdude-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avrdude-dev
Reply | Threaded
Open this post in threaded view
|

[patch #9573] patch to add atmelice avr32 programming support to avrdude

Kevin Cuzner-2
Follow-up Comment #1, patch #9573 (project avrdude):

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.

    _______________________________________________________

Reply to this item at:

  <http://savannah.nongnu.org/patch/?9573>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.nongnu.org/


_______________________________________________
avrdude-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/avrdude-dev
Reply | Threaded
Open this post in threaded view
|

[patch #9573] patch to add atmelice avr32 programming support to avrdude

Kevin Cuzner-2
Update of patch #9573 (project avrdude):

                  Status:                    None => Postponed              
             Assigned to:                    None => joerg_wunsch          

    _______________________________________________________

Follow-up 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.


 programmer
+  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;
+#endif
     }


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)) {
       avrdude_message(MSG_INFO,
-                     "%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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9573>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


Reply | Threaded
Open this post in threaded view
|

[patch #9573] patch to add atmelice avr32 programming support to avrdude

Kevin Cuzner-2
Follow-up Comment #3, patch #9573 (project avrdude):

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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9573>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/