Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg

[panic]

Hi,

thanks for the first reply to any of my reported issues!

Klaus Rudolph:
> I only have taken a very short look over your mail. But it sounds not
> very good to me because replacing a clear templated structure with: void
> pointers, c macros, casting, exclude stuff for swig, etc

- I'd consider "clear templated structure" debatable. IMHO, it's a
confusing cyclic structure, for example, having IOReg<HWPort> pin_reg as
a member of HWPort.
- Also, instantiating a template class for every Hardware/peripheral
just because of a callback seems wrong to me. It makes it impossible to
cast the register to sth else without knowing the template parameter/HW
type (see below).
- void*: I also don't like it, but the IOReg is given "this" as its
"owner" and the proper callback handlers in the constructor. The static
callback handlers are /private/ class methods, only visible from within
the same class. I don't see many opportunities to mix the types of
"this" and the callbacks.
- c macros: they are an easy way to generate the g/setters. If there's
another way, I'm open to it. Recent GCC (e.g. Debian stable: 4.9.2)
generates nice warnings/errors and also cites the macro which was
expanded and resulted in the error. I also did not want to rewrite all
the object s/getter methods, thus, only wrapping them in a static class
method. Using the macro for standard unsigned char getter/setters, IMHO,
keeps the code quite readable.
- exclude stuff for swig: the callback handlers aren't needed for swig,
not intended to be ever called from swig, nor does swig like void*.
Naturally then, these methods should be excluded from swig.


> Sorry that I have very very less time to dig into your proposal, but
> only from the given excerpt here in the mail I tend to NOT change the
> existing code.

well, the motivation to change IOReg is actually a bit more complex:
Current IOReg<P> does only provide set and get methods (via
RWMemoryMember). The bitwise SBI/CBI instructions are implemented as
read/modify/write in AvrDevice, which leads to a severe bug in
simulation: the lower 0x20 IO registers are bitwise accessible and RMW
is just wrong.

Example:
SBI of bit 0 in PINB register should toggle (xor) PORTB.PB0. Using RMW
approach, we read PINB = 0xff (because DDRB is input/tristate, read as
'1' for each pin), we OR the bitmask (1<<0)=1 on top of it (=0xff) and
write it back to PINB, *which toggles all PORTB bits* --> bug.

To fix it, I wanted to add a set_bit() method to IOReg<P>. In AvrDevice,
I then cast RWMemoryMember to IOReg:

> bool AvrDevice::SetIORegBit(unsigned addr, unsigned bitaddr, bool bval) {
>     IOReg<XXX> *reg = dynamic_cast<IOReg<XXX>*>(rw[addr + registerSpaceSize]);
>     reg->set_bit(bitaddr, bval);
>     return true;
> }

But I cannot do this because there's no way to deduce the template
parameter "XXX". So, template is bad... removing the template is a solution.

For completeness, here's attached the patch to fix the actual bug, after
the template is removed/assuming previous patch is applied.

- it introduces a new class "BitAccessibleRegister" with a default
  implementation using RMW for set_bit()
- makes registers in the IO memory space children of
  BitAccessibleRegister (e.g. GPIORegister, CPKPRRegister)
  for new hierarchy, see attached PNG.
- it fixes PORTx and DDRx registers not being traces correctly
- fixes PINx write to toggle PORTx using single-bit SBI/CBI instructions

Cheers,
panic

0001-fix-single-bit-access-to-PINx-and-I-O-registers.patch
Description: Text Data

classRWMemoryMember__inherit__graph-without_template-with_bitaccessible.png
Description: PNG image