bugAVR Downloader/UploaDEr - Bugs: bug #34302, Feature request : device...

 
 

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

bug #34302: Feature request : device configuration with parent classes

Submitter:  jonathan nifenecker <crazyiop>
Submitted:  Fri 16 Sep 2011 10:00:57 AM UTC
Votes: 150
 
Category:  None Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Privacy:  Public
Assigned to:  rliebscher Originator Name:  crazyiop
Open/Closed:  Closed Release:  None
Programmer hardware:  Device type: 

Jump to the original submission

  Spam posted by mrlove2
Fri 27 Jan 2012 09:20:57 PM UTC, comment #11: 

Now in main.c parts starting with '.' are not longer listed as valid parts for the -p option.

This way one can define "abstract" parts as base for several other devices. See xmegas definitions in current svn which base on ".xmega".

Rene Liebscher <rliebscher>
Group Member
Thu 19 Jan 2012 08:07:55 AM UTC, comment #10: 

Thanks Rene. Yes, I forgot to free the old definition after removing it from the list.

I certainly agree that freeing allocated memory at exit is good form.

Tomaž Šolc <avian>
Tue 17 Jan 2012 09:01:42 PM UTC, comment #9: 

This is added in svn revision 1041.

I also free the now unused memory definition, as this would be lost memory.

Moreover I added some free functions for parts and programmers and use them at exit. This would not be really necessary as this is done by the OS, but so valgrind reports no left memory, and it becomes easier to spot any memory leaks and other potential problems.

Rene Liebscher <rliebscher>
Group Member
Sat 14 Jan 2012 05:43:05 PM UTC, comment #8: 

Rene, I agree that the first method is better. It's also quite simple to implement - see the attached patch.

(file #24820)

Tomaž Šolc <avian>
Mon 02 Jan 2012 04:20:16 PM UTC, comment #7: 

There is still some work.

For example for bug #21797 I tried to redefine only the flash memory. But this ends in having two flash memory entries in the memory table.

         AVR Part                      : AT90PWM316
         Chip Erase delay              : 9000 us
         PAGEL                         : PD8
         BS2                           : PE2
         RESET disposition             : possible i/o
         RETRY pulse                   : SCK
         serial program mode           : yes
         parallel program mode         : yes
         Timeout                       : 200
         StabDelay                     : 100
         CmdexeDelay                   : 25
         SyncLoops                     : 32
         ByteDelay                     : 0
         PollIndex                     : 3
         PollValue                     : 0x53
         Memory Detail                 :

                                  Block Poll               Page                       Polled
           Memory Type Mode Delay Size  Indx Paged  Size   Size #Pages MinW  MaxW   ReadBack
           ----------- ---- ----- ----- ---- ------ ------ ---- ------ ----- ----- ---------
           eeprom        65     6     4    0 no        512    4      0  4000  4500 0xff 0xff
           flash         65     6    64    0 yes      8192   64    128  4500  4500 0xff 0xff
           signature      0     0     0    0 no          3    0      0     0     0 0x00 0x00
           lock           0     0     0    0 no          1    0      0  9000  9000 0x00 0x00
           lfuse          0     0     0    0 no          1    0      0  9000  9000 0x00 0x00
           hfuse          0     0     0    0 no          1    0      0  9000  9000 0x00 0x00
           efuse          0     0     0    0 no          1    0      0  9000  9000 0x00 0x00
           calibration    0     0     0    0 no          1    0      0     0     0 0x00 0x00
           flash         33     6   128    0 yes     16384  128    128  4500  4500 0xff 0xff


It is clear the new memory must replace the old one. But it must be defined if this replaces the whole memory block (and you have repeat all settings for the block) or if the original block is duplicated and new settings just overwrite the old ones in the block.

I would prefer the first method as there are currently no means to delete unwanted settings in a duplicated block (opcodes, ...) Also the repeating of all settings would still work if later the second method is implemented.

Any comments?

Rene Liebscher <rliebscher>
Group Member
Thu 29 Dec 2011 04:59:24 PM UTC, comment #6: 

Implemented in svn revision 1028.

But added duplicating of opcodes. (This results in same memory consumption as it was before adding the part parent feature.)

As it is integrated now, it can be used in the config file.

Rene Liebscher <rliebscher>
Group Member
Wed 21 Dec 2011 02:49:52 PM UTC, comment #5: 

Reply to comment #3:

This warning was added because the config file before svn revision 1023 had a part which defined some operations twice. This was the reason for a memory leak and if you have the change to report such errors in the config file you should use it.

However, if opcodes get removed than there is also no need to duplicate them ;-)
(And if they do not get removed, then duplicated opcodes just need the same amount of memory as the current state of avrdude.)
May be using a preliminary solution with potential memory leaks might be acceptable, when we get so the parent part feature and its smaller config file now.

Reply to comment #4:

Restarting the parser is done right now if you have a system wide and a user config file, in this case you have two calls of read_config().
(For a clean new start one would just have to delete and free all parts and programmers from the global lists.)

Rene Liebscher <rliebscher>
Group Member
Wed 21 Dec 2011 02:29:19 PM UTC, comment #4: 

I think, right now, it's completely safe to assume that an
application will parse the configuration file exactly once.
Sure, adding a destructor would be the politically correct
thing to have, but there's more than that: restarting a yacc
parser would require additional precautions anyway.

As for the opcodes, I think the best thing would be to drop
them alltogether.  Atmel has never changed them, and if you
look at the Atmel tools like AVRISP or STK500, they don't
get fed an array of opcodes as part of their per-device
configuration.  Consequently, the opcodes must be tied into
firmware there, and cannot be changed.  Dropping the opcodes
would be a step towards the ability to generate the
avrdude.conf entry completely from the XML device data (which
don't have any notion of an ISP opcode either).

Joerg Wunsch <joerg_wunsch>
Group administrator
Wed 21 Dec 2011 01:59:58 PM UTC, comment #3: 

I think just removing the following would work.

      if (current_part->op[opnum] != NULL) {
        fprintf(stderr,
              "%s: warning at %s:%d: operation redefined\n",
              progname, infile, lineno);
        free(current_part->op[opnum]);
      }

That warning doesn't make sense when you are redefining things in derived parts anyway. It would create a small memory leak in case an opcode gets redefined inside in a part itself though (not parts derived from it).

If that's not acceptable, I can do refcounting in OPCODE. It think that's better than just duplicating everything.

Tomaž Šolc <avian>
Wed 21 Dec 2011 12:33:07 PM UTC, comment #2: 

The patch needs also an avr_dup_opcode() function to duplicate the
op-array (OPCODE op[AVR_OP_MAX]; / opcodes */) in AVRPART and AVRMEM structures.

If you just copy the array without to duplicate the entries you get a problem when someone defines an op code for the new part different than its parent.


BTW: An avr_part_free function would be nice in case anyone uses libavrdude and reads (several) times a config file and wants later  to free all the stuff that was created at reading the config file.

Rene Liebscher <rliebscher>
Group Member
Thu 17 Nov 2011 07:55:12 AM UTC, comment #1: 

The attached patch implements this feature.

The syntax is slightly different from the one proposed, because it greatly simplifies the code if we can parse the parent part ID before any other part parameters.

(file #24389)

Tomaž Šolc <avian>
Fri 16 Sep 2011 10:00:57 AM UTC, original submission:  

As it was discussed earlier in the mailing list and is imho a good idea, I put it here so it will not be forgoten.
from http://lists.gnu.org/archive/html/avrdude-dev/2011-08/msg00059.html

a part is derived from another part name. any additional configuration overrides the original configuration. if there's no additional configuration, it's exactly the same except as an aliased name.
this is more of a class extension concept rather than an alias.

this has the potential to significantly compact avrdude.conf as there are numerous chips which are nearly identical except for a few differences. (memory size, signature for P/PA/PV extension, etc)


[part]
parent = [parent]
[configuration that adds, or replaces, information derived from the parent]

jonathan nifenecker <crazyiop>

 

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

Attached Files
file #24389:  part_parent.patch added by avian (6KiB - text/x-patch - Implement part parent syntax in avrdude.conf)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mrlove2 (Posted a comment)
  • -email is unavailable- added by joerg_wunsch (Posted a comment)
  • -email is unavailable- added by rliebscher (Posted a comment)
  • -email is unavailable- added by seventhguardian (Voted in favor of this item)
  • -email is unavailable- added by rliebscher (Voted in favor of this item)
  • -email is unavailable- added by avian (Updated the item)
  • -email is unavailable- added by crazyiop (Submitted the item)
  •  

    There are 150 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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-09-03 rliebscher StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2012-01-14 avian Attached File- Added override_memory_entries.patch, #24820
    2011-12-29 rliebscher StatusNone Ready For Test
        Assigned toNone rliebscher
    2011-12-21 seventhguardian Carbon-Copy- Added seventhguardian
    2011-12-13 rliebscher Carbon-Copy- Added rliebscher
    2011-11-17 avian Attached File- Added part_parent.patch, #24389

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code