[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Simulavr-devel] [PATCH] use static callbacks instead of template pa
From: |
panic |
Subject: |
Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg |
Date: |
Fri, 16 Jun 2017 22:48:00 +0000 |
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
- [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, panic, 2017/06/15
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, Klaus Rudolph, 2017/06/16
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg,
panic <=
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, panic, 2017/06/16
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, Klaus Rudolph, 2017/06/17
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, panic, 2017/06/17
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, Michael Hennebry, 2017/06/17
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, panic, 2017/06/17
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, panic, 2017/06/17
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, panic, 2017/06/17
- [Simulavr-devel] SBI vs. PINx and interrupt flag registers, Michael Hennebry, 2017/06/19
- Re: [Simulavr-devel] SBI vs. PINx and interrupt flag registers, panic, 2017/06/19
- Re: [Simulavr-devel] [PATCH] use static callbacks instead of template param for IOReg, Michael Hennebry, 2017/06/19