Re: [Simulavr-devel] Uniform the code

[Michael N. Moran]

Hi Knut,

Since I am the one who originally submitted the
atmega48 patch, I'll try to address this.

Knut Schwichtenberg wrote:
> first of all I must clarify I'm NOT a C++ specialist.

I write embedded C++ code most days... so I can probably
help to clarify my choices.

> Currently there is a problem with the atmega48 which
> leads to segmentation fault. 

Yes, I've yet to get the simulavrxx to work from CVS,
since Joel added my patches. Unfortunately, I haven't
had much time to diagnose what is happening.

> While M128 is okay M48 should be simplified.

I'm not sure what you mean by simplify.

> But the module looks fully different! I expect it works
> the same, but why is it programmed different?

From what I can see from your examples below, I
guess that you have issue with my use of member
variables that are objects rather than pointers
to objects.

As an outgrowth of my background in embedded systems,
I generally try to avoid the use of dynamic memory
allocation. In this case it is mostly a matter of
avoiding common errors:

1) The compiler will complain if a member variable
is not properly initialized. From the compiler's point-
of-view, pointers do not require initialization, and
therefore runtime errors can result if the developer
forgets to initialize the pointer.

2) Memory leaks happen if member pointers are not
explicitly deleted in the class destructor. Again,
this is another source of run-time error that is
avoided by using member objects rather than member
pointers-to-objects.

> Normally everyone could say if it works, okay who cares
> about the way it is hacked, but just these modules have
> to be created for each CPU and we are at the very
> beginning and now it is the time to decide M128 way or
> M48 way or fully different. 

I guess you now know how *I* would vote ;)
However, I'm currently at a disadvantage in terms
of precedence and *working* code ... I'll try to remedy
that soon.

In my defense, I will say that when I submitted this
code in 2005, it worked nicely. I suppose some kind
of bit-rot happened between then and the time it was
finally merged last month. :)

> If this is not fixed now, maintenance in the future will
> become a nightmare.

I think this is a bit of an over-reaction.

> I'll take some examples to clarify:
> atmega128.cpp:
>
> AvrDevice_atmega128::AvrDevice_atmega128():
> AvrDevice(224, 4096, 0xef00, 128*1024),
> aref()
> {
>         irqSystem = new HWIrqSystem(this, 4); //4 bytes per vector
>         eeprom = new HWMegaEeprom(this, irqSystem, 4096, 22);
>         stack = new HWStack(this, Sram, 0xffff);
>         porta= new HWPort(this, "A");         <-- explicit assignment

If you look at the atmega128 header file, you will see
that the member variables (e.g. irqSystem, eeprom, stack,
and porta) are pointers. As a result, them must be
initialized using "new" in the constructor.

> ...
>
> AvrDevice_atmega48::AvrDevice_atmega48():
> AvrDevice(      224,    // I/O space above General Purpose Registers
>                         512,    // RAM size
>                         0,      // External RAM size
>                         4*1024  // Flash Size
>                         ),
> aref(),
> adc6(),
> adc7(),
> portb(this,"B"),                         <--- Initial assignment
> portc(this,"C"),
> portd(this,"D"),
> prescaler(this),
> admux(  this,
>                 &portc.GetPin(0),
>                 &portc.GetPin(1),
>                 &portc.GetPin(2),
>                 &portc.GetPin(3),
>                 &portc.GetPin(4),
>                 &portc.GetPin(5),
>                 &adc6,
>                 &adc7
>                 )
> {
In this case, the member variables are being initialized
in the (aptly named) initializer list. There is no dynamic
memory allocation using new. The constructors of the
individual member objects are satisfied here, and the
memory for the objects is a part of the class structure
rather than coming from the heap.


> Personally I like explicit assignment because of my old
> age understanding.

My old age (52 years) respectfully disagrees ;)

> The differences of the way the variables are assigned
> leads to a different assignment of the physical address
> to the name:
>
> m128:
>         rw[0x32]= new RWPort(this, portd);
>
> m48:
>         rw[0x2B]= new RWPort(this, &portd);

The constructor of RWPort requires a pointer. The member
variable portd is a pointer in the M128 case and an object
in the M48 case. Thus, to satisfy the constructor of
the RWPort, the address-of operator needs to be used
in the M48 case.

> The difference 0x32 / 0x28 is Atmel based, but the portd
> vs. &portd is selfmade and that's not good.

I think this is simply a lack of familiarity with C++
rather than a matter of good vs evil. While I would agree
that consistency is a good thing, it is not the *only*
thing.

> There should be one unique way how the atmegaxxx /
> attinyxxx modules will be created! Please discuss.

Ultimately, this is a matter of style, but I believe
there are good reasons for my atmega48 choices. ymmv.

--
Michael N. Moran           (h) 770 516 7918
5009 Old Field Ct.         (c) 678 521 5460
Kennesaw, GA, USA 30144    http://mnmoran.org

"So often times it happens, that we live our lives in chains
 and we never even know we have the key."
"Already Gone" by Jack Tempchin (recorded by The Eagles)

The Beatles were wrong: 1 & 1 & 1 is 1