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

 
 

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

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

Submitted by:  jonathan nifenecker <crazyiop>
Submitted on:  Fri 16 Sep 2011 10:00:57 AM UTC  
Votes:  150  
 
Category: NoneSeverity: 3 - Normal
Priority: 5 - NormalItem Group: None
Status: FixedPrivacy: Public
Assigned to: Rene Liebscher <rliebscher>Originator Name: crazyiop
Open/Closed: Closed

(Jump to the original submission 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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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.

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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project 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>
Project MemberIn charge of this item.
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>

 

Attached Files
file #24389:  part_parent.patch added by avian (6kB - 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
  • -unavailable- added by mrlove2 (Posted a comment)
  • -unavailable- added by joerg_wunsch (Posted a comment)
  • -unavailable- added by rliebscher (Posted a comment)
  • -unavailable- added by seventhguardian (Voted in favor of this item)
  • -unavailable- added by rliebscher (Voted in favor of this item)
  • -unavailable- added by avian (Updated the item)
  • -unavailable- added by crazyiop (Submitted the item)
  •  

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

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 03 Sep 2013 08:06:48 PM UTCrliebscherStatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Sat 14 Jan 2012 05:43:05 PM UTCavianAttached File-=>Added override_memory_entries.patch, #24820
    Thu 29 Dec 2011 04:59:24 PM UTCrliebscherStatusNone=>Ready For Test
      Assigned toNone=>rliebscher
    Wed 21 Dec 2011 11:59:15 AM UTCseventhguardianCarbon-Copy-=>Added seventhguardian
    Tue 13 Dec 2011 01:35:21 PM UTCrliebscherCarbon-Copy-=>Added rliebscher
    Thu 17 Nov 2011 07:55:12 AM UTCavianAttached File-=>Added part_parent.patch, #24389

    Back to the top


    Powered by Savane 3.1-cleanup