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
>From 11f5325bf38d8bf9c88c1c37af0928f23f4eaa56 Mon Sep 17 00:00:00 2001
From: panic <li...@xandea.de>
Date: Mon, 12 Jun 2017 23:34:33 +0200
Subject: [PATCH] fix single-bit access to PINx and I/O registers
---
src/avrdevice.cpp | 8 +--
src/hwport.cpp | 15 ++++-
src/hwport.h | 3 +
src/hwsreg.h | 4 +-
src/rwmem.cpp | 27 ++++++--
src/rwmem.h | 187 +++++++++++++++++++++++++++++------------------------
src/specialmem.cpp | 8 +--
src/specialmem.h | 8 +--
8 files changed, 154 insertions(+), 106 deletions(-)
diff --git a/src/avrdevice.cpp b/src/avrdevice.cpp
index 51098cb..11ea1f3 100644
--- a/src/avrdevice.cpp
+++ b/src/avrdevice.cpp
@@ -473,12 +473,8 @@ bool AvrDevice::SetIOReg(unsigned addr, unsigned char val) {
bool AvrDevice::SetIORegBit(unsigned addr, unsigned bitaddr, bool bval) {
assert(addr < 0x20); // only first 32 IO registers are bit-settable
- unsigned char val = *(rw[addr + registerSpaceSize]);
- if(bval)
- val |= 1 << bitaddr;
- else
- val &= ~(1 << bitaddr);
- *(rw[addr + registerSpaceSize]) = val;
+ BitAccessibleRegister *reg = dynamic_cast<BitAccessibleRegister*>(rw[addr + registerSpaceSize]);
+ reg->set_bit(bitaddr, bval);
return true;
}
diff --git a/src/hwport.cpp b/src/hwport.cpp
index 77c3a16..2d430f6 100644
--- a/src/hwport.cpp
+++ b/src/hwport.cpp
@@ -40,7 +40,7 @@ HWPort::HWPort(AvrDevice *core, const string &name, bool portToggle, int size):
port_reg(this, "PORT",
this, &HWPort::GetPort, &HWPort::SetPort),
pin_reg(this, "PIN",
- this, &HWPort::GetPin, &HWPort::SetPin),
+ this, &HWPort::GetPin, &HWPort::SetPin, &HWPort::SetPinBit),
ddr_reg(this, "DDR",
this, &HWPort::GetDdr, &HWPort::SetDdr)
{
@@ -103,6 +103,9 @@ void HWPort::CalcOutputs(void) { // Calculate the new output value to be transmi
pintrace[actualBitNo]->change(p[actualBitNo].outState);
}
pin = tmpPin;
+
+ port_reg.hardwareChange(port);
+ ddr_reg.hardwareChange(ddr);
pin_reg.hardwareChange(pin);
}
@@ -124,4 +127,14 @@ void HWPort::SetPin(unsigned char val) {
avr_warning("Writing of 'PORT%s.PIN' (with %d) is not supported.", myName.c_str(), val);
}
+void HWPort::SetPinBit(unsigned char bitpos, bool val) {
+ unsigned char v = (val ? 1 : 0) << bitpos;
+ if (portToggleFeature) {
+ port ^= v;
+ CalcOutputs();
+ port_reg.hardwareChange(port);
+ } else
+ avr_warning("Writing of 'PORT%s.PIN' (with %d) is not supported.", myName.c_str(), v);
+}
+
/* EOF */
diff --git a/src/hwport.h b/src/hwport.h
index 42e6acd..571553f 100644
--- a/src/hwport.h
+++ b/src/hwport.h
@@ -60,6 +60,9 @@ class HWPort: public Hardware, public TraceValueRegister {
IOREG_SETGET_UCHAR(HWPort, Port)
IOREG_SETGET_UCHAR(HWPort, Ddr)
IOREG_SETGET_UCHAR(HWPort, Pin)
+ static void SetPinBit(void *this_ptr, unsigned char bitpos, bool bval) {
+ static_cast<HWPort*>(this_ptr)->SetPinBit(bitpos, bval);
+ }
#endif
public:
diff --git a/src/hwsreg.h b/src/hwsreg.h
index c1975e7..e76b725 100644
--- a/src/hwsreg.h
+++ b/src/hwsreg.h
@@ -62,10 +62,10 @@ class HWSreg: public HWSreg_bool {
\todo Replace the status register with an ordinary byte somewhere and simple
inline access functions sN(), gN() to get/set flags. This should also make
accesses faster. */
-class RWSreg: public RWMemoryMember {
+class RWSreg: public BitAccessibleRegister {
public:
- RWSreg(TraceValueRegister *registry, HWSreg *s): RWMemoryMember(registry, "SREG"), status(s) {}
+ RWSreg(TraceValueRegister *registry, HWSreg *s): BitAccessibleRegister(registry, "SREG"), status(s) {}
//! reflect a change, which comes from CPU core
void trigger_change(void) { tv->change((int)*status); }
diff --git a/src/rwmem.cpp b/src/rwmem.cpp
index 8309af3..e174f3e 100644
--- a/src/rwmem.cpp
+++ b/src/rwmem.cpp
@@ -91,9 +91,24 @@ RWMemoryMember::~RWMemoryMember() {
delete tv;
}
+BitAccessibleRegister::BitAccessibleRegister()
+ : RWMemoryMember() {}
+
+BitAccessibleRegister::BitAccessibleRegister(
+ TraceValueRegister *registry,
+ const std::string &tracename)
+ : RWMemoryMember(registry, tracename) {}
+
+void BitAccessibleRegister::set_bit(unsigned char bitpos, bool bval) {
+ if (bval)
+ set(get() | (1<<bitpos));
+ else
+ set(get() & ~(1<<bitpos));
+}
+
CLKPRRegister::CLKPRRegister(AvrDevice *core,
TraceValueRegister *registry):
- RWMemoryMember(registry, "CLKPR"),
+ BitAccessibleRegister(registry, "CLKPR"),
Hardware(core),
_core(core) {
if(_core->fuses->GetFuseBit(AvrFuses::FB_CKDIV8))
@@ -142,7 +157,7 @@ void CLKPRRegister::set(unsigned char v) {
XDIVRegister::XDIVRegister(AvrDevice *core,
TraceValueRegister *registry):
- RWMemoryMember(registry, "XDIV"),
+ BitAccessibleRegister(registry, "XDIV"),
Hardware(core)
{
Reset();
@@ -168,7 +183,7 @@ void XDIVRegister::set(unsigned char v) {
OSCCALRegister::OSCCALRegister(AvrDevice *core,
TraceValueRegister *registry,
int cal):
- RWMemoryMember(registry, "OSCCAL"),
+ BitAccessibleRegister(registry, "OSCCAL"),
Hardware(core),
cal_type(cal)
{
@@ -211,7 +226,7 @@ unsigned char RAM::get() const { return value; }
void RAM::set(unsigned char v) { value=v; }
InvalidMem::InvalidMem(AvrDevice* _c, int _a):
- RWMemoryMember(),
+ BitAccessibleRegister(),
core(_c),
addr(_a) {}
@@ -232,7 +247,7 @@ void InvalidMem::set(unsigned char c) {
}
NotSimulatedRegister::NotSimulatedRegister(const char * message_on_access_)
- : message_on_access(message_on_access_) {}
+ : BitAccessibleRegister(), message_on_access(message_on_access_) {}
unsigned char NotSimulatedRegister::get() const {
avr_warning("%s (read from register)", message_on_access);
@@ -244,7 +259,7 @@ void NotSimulatedRegister::set(unsigned char c) {
}
IOSpecialReg::IOSpecialReg(TraceValueRegister *registry, const std::string &name):
- RWMemoryMember(registry, name)
+ BitAccessibleRegister(registry, name)
{
Reset();
}
diff --git a/src/rwmem.h b/src/rwmem.h
index 3168d2b..3d194ee 100644
--- a/src/rwmem.h
+++ b/src/rwmem.h
@@ -87,15 +87,111 @@ class RWMemoryMember {
const bool isInvalid;
};
+//! I/O Registers within the I/O address range from 0x00 to 0x1F are directly
+//! bit accessible via the SBI and CBI instructions.
+class BitAccessibleRegister : public RWMemoryMember {
+ public:
+ explicit BitAccessibleRegister();
+ explicit BitAccessibleRegister(
+ TraceValueRegister *registry,
+ const std::string &tracename);
+
+ //! set or clear a single bit in a register.
+ //! By default, this emulates a full read/modify/write cycle.
+ virtual void set_bit(unsigned char bitpos, bool bval);
+ // TODO maybe we can also provide default implementations for get()
+ // and set() here, instead of duplicating them every time
+};
+
+//! helper for IOReg static callback wrappers
+#define IOREG_SET_UCHAR(cls, reg) \
+ static void Set##reg(void *this_ptr, unsigned char val) \
+ { static_cast<cls*>(this_ptr)->Set##reg(val); }
+
+#define IOREG_GET_UCHAR(cls, reg) \
+ static unsigned char Get##reg(void *this_ptr) \
+ { return static_cast<cls*>(this_ptr)->Get##reg(); }
+
+#define IOREG_SETGET_UCHAR(cls, reg) \
+ IOREG_SET_UCHAR(cls, reg) \
+ IOREG_GET_UCHAR(cls, reg)
+
+//! IO register to be specialized for a certain class/hardware
+//! The class accepts static member function pointers of the owner/parent of
+//! the IO register.
+class IOReg: public BitAccessibleRegister {
+ public:
+ typedef unsigned char(*getter_t)(void*);
+ typedef void (*setter_t)(void*, unsigned char);
+ typedef void (*setter_bit_t)(void*, unsigned char, bool);
+ /*! Creates an IO control register for controlling hardware units
+ \param _p: pointer to object this will be part of
+ \param _g: pointer to get method
+ \param _s: pointer to set method
+ \param _sb: pointer to single-bit set method */
+ explicit IOReg(TraceValueRegister *registry,
+ const std::string &tracename,
+ void *_p,
+ getter_t _g,
+ setter_t _s,
+ setter_bit_t _sb=0)
+ : BitAccessibleRegister(registry, tracename),
+ p(_p), g(_g), s(_s), sb(_sb)
+ {
+ // 'undefined state' doesn't really make sense for IO registers
+ if (tv)
+ tv->set_written();
+ }
+
+ /*! Reflects a value change from hardware (for example timer count occured)
+ @param val the new register value */
+ void hardwareChange(unsigned char val) { if(tv) tv->change(val); }
+ /*! Releases the TraceValue to hide this IOReg from registry */
+ void releaseTraceValue(void) {
+ if(tv) {
+ registry->UnregisterTraceValue(tv);
+ delete tv;
+ tv = NULL;
+ }
+ }
+
+ protected:
+ unsigned char get() const {
+ return g(p);
+ }
+ void set(unsigned char val) {
+ s(p, val);
+ }
+ public:
+ void set_bit(unsigned char bitpos, bool bval) {
+ // use callback if set, otherwise emulate read/modify/write
+ if (sb) {
+ sb(p, bitpos, bval);
+ } else {
+ if (bval)
+ set(get() | (1 << bitpos));
+ else
+ set(get() & ~(1 << bitpos));
+ }
+ }
+
+ private:
+ void *p;
+ getter_t g;
+ setter_t s;
+ setter_bit_t sb;
+};
+
+
//! A register in IO register space unrelated to any peripheral. "GPIORx" in datasheets.
/*! Allows clean read and write accesses and simply has one stored byte. */
-class GPIORegister: public RWMemoryMember, public Hardware {
+class GPIORegister: public BitAccessibleRegister, public Hardware {
public:
GPIORegister(AvrDevice *core,
TraceValueRegister *registry,
const std::string &tracename):
- RWMemoryMember(registry, tracename),
+ BitAccessibleRegister(registry, tracename),
Hardware(core)
{ value = 0; }
@@ -111,7 +207,7 @@ class GPIORegister: public RWMemoryMember, public Hardware {
};
//! Implement CLKPR register
-class CLKPRRegister: public RWMemoryMember, public Hardware {
+class CLKPRRegister: public BitAccessibleRegister, public Hardware {
public:
CLKPRRegister(AvrDevice *core,
@@ -132,7 +228,7 @@ class CLKPRRegister: public RWMemoryMember, public Hardware {
};
//! Implement XDIV register
-class XDIVRegister: public RWMemoryMember, public Hardware {
+class XDIVRegister: public BitAccessibleRegister, public Hardware {
public:
XDIVRegister(AvrDevice *core,
@@ -150,7 +246,7 @@ class XDIVRegister: public RWMemoryMember, public Hardware {
};
//! Implement OSCCAL register
-class OSCCALRegister: public RWMemoryMember, public Hardware {
+class OSCCALRegister: public BitAccessibleRegister, public Hardware {
public:
// select type of oscillator, see AVR053 application note!
@@ -197,7 +293,7 @@ class RAM : public RWMemoryMember {
//! Memory on which access should be avoided! :-)
/*! All accesses to this type of memory will produce an error. */
-class InvalidMem : public RWMemoryMember {
+class InvalidMem : public BitAccessibleRegister {
private:
AvrDevice* core;
int addr;
@@ -212,7 +308,7 @@ class InvalidMem : public RWMemoryMember {
//! An IO register which is not simulated because programmers are lazy.
/*! Reads and writes are ignored and produce warning. */
-class NotSimulatedRegister : public RWMemoryMember {
+class NotSimulatedRegister : public BitAccessibleRegister {
private:
const char * message_on_access;
@@ -224,81 +320,6 @@ class NotSimulatedRegister : public RWMemoryMember {
void set(unsigned char);
};
-//! helper for IOReg static callback wrappers
-#define IOREG_SET_UCHAR(cls, reg) \
- static void Set##reg(void *this_ptr, unsigned char val) \
- { static_cast<cls*>(this_ptr)->Set##reg(val); }
-
-#define IOREG_GET_UCHAR(cls, reg) \
- static unsigned char Get##reg(void *this_ptr) \
- { return static_cast<cls*>(this_ptr)->Get##reg(); }
-
-#define IOREG_SETGET_UCHAR(cls, reg) \
- IOREG_SET_UCHAR(cls, reg) \
- IOREG_GET_UCHAR(cls, reg)
-
-//! IO register to be specialized for a certain class/hardware
-//! The class accepts static member function pointers of the owner/parent of
-//! the IO register.
-class IOReg: public RWMemoryMember {
-
- public:
- typedef unsigned char(*getter_t)(void*);
- typedef void (*setter_t)(void*, unsigned char);
- /*! Creates an IO control register for controlling hardware units
- \param _p: pointer to object this will be part of
- \param _g: pointer to get method
- \param _s: pointer to set method */
- IOReg(TraceValueRegister *registry,
- const std::string &tracename,
- void *_p,
- getter_t _g=0,
- setter_t _s=0):
- RWMemoryMember(registry, tracename),
- p(_p),
- g(_g),
- s(_s)
- {
- // 'undefined state' doesn't really make sense for IO registers
- if (tv)
- tv->set_written();
- }
-
- /*! Reflects a value change from hardware (for example timer count occured)
- @param val the new register value */
- void hardwareChange(unsigned char val) { if(tv) tv->change(val); }
- /*! Releases the TraceValue to hide this IOReg from registry */
- void releaseTraceValue(void) {
- if(tv) {
- registry->UnregisterTraceValue(tv);
- delete tv;
- tv = NULL;
- }
- }
-
- protected:
- unsigned char get() const {
- if (g)
- return g(p);
- else if (tv) {
- avr_warning("Reading of '%s' is not supported.", tv->name().c_str());
- }
- return 0;
- }
- void set(unsigned char val) {
- if (s)
- s(p, val);
- else if (tv) {
- avr_warning("Writing of '%s' (with %d) is not supported.", tv->name().c_str(), val);
- }
- }
-
- private:
- void *p;
- getter_t g;
- setter_t s;
-};
-
class IOSpecialReg;
//! Interface class to connect hardware units to control registers
@@ -337,7 +358,7 @@ class IOSpecialRegClient {
//! IO register, which holds configuration for more than one hardware unit
//! \todo Set method could modify value, how to reflect it on TraceValue mechanism?
//! Same problem for the get method.
-class IOSpecialReg: public RWMemoryMember {
+class IOSpecialReg: public BitAccessibleRegister {
public:
//! Creates a IOSpecialReg instance, see RWMemoryMember for more info
diff --git a/src/specialmem.cpp b/src/specialmem.cpp
index b3411eb..66092cf 100644
--- a/src/specialmem.cpp
+++ b/src/specialmem.cpp
@@ -33,7 +33,7 @@ using namespace std;
RWWriteToFile::RWWriteToFile(TraceValueRegister *registry,
const string &tracename,
const string &filename):
- RWMemoryMember(registry, tracename),
+ BitAccessibleRegister(registry, tracename),
os(filename=="-" ? cout : ofs)
{
if(filename != "-")
@@ -53,7 +53,7 @@ unsigned char RWWriteToFile::get() const {
RWReadFromFile::RWReadFromFile(TraceValueRegister *registry,
const string &tracename,
const string &filename):
- RWMemoryMember(registry, tracename),
+ BitAccessibleRegister(registry, tracename),
is((filename=="-") ? cin : ifs)
{
if(filename != "-")
@@ -73,7 +73,7 @@ unsigned char RWReadFromFile::get() const {
RWExit::RWExit(TraceValueRegister *registry,
const string &tracename) :
- RWMemoryMember(registry, tracename) {}
+ BitAccessibleRegister(registry, tracename) {}
void RWExit::set(unsigned char c) {
@@ -91,7 +91,7 @@ unsigned char RWExit::get() const {
RWAbort::RWAbort(TraceValueRegister *registry,
const string &tracename) :
- RWMemoryMember(registry, tracename) {}
+ BitAccessibleRegister(registry, tracename) {}
void RWAbort::set(unsigned char c) {
avr_warning("Aborting at simulated program request (write)");
diff --git a/src/specialmem.h b/src/specialmem.h
index 155faa4..e7935a1 100644
--- a/src/specialmem.h
+++ b/src/specialmem.h
@@ -36,7 +36,7 @@
/*! Memory register which will redirect all write
accesses to the given (FIFO) file. The output
format in the file is binary. */
-class RWWriteToFile: public RWMemoryMember {
+class RWWriteToFile: public BitAccessibleRegister {
public:
/*! The output filename can be '-' which will
make this object use cout then. */
@@ -55,7 +55,7 @@ class RWWriteToFile: public RWMemoryMember {
/*! Memory register which will fulfill all reads with
a byte drawn from a given (FIFO) file. The input
format is binary. */
-class RWReadFromFile: public RWMemoryMember {
+class RWReadFromFile: public BitAccessibleRegister {
public:
/*! The input filename can be '-' which will
make this object use cin then. */
@@ -74,7 +74,7 @@ class RWReadFromFile: public RWMemoryMember {
/*! Any access to this memory will exit the simulator.
If a byte is written, it will be return code of the simulavr
process. If a byte is being read, the exit code is 0x00. */
-class RWExit: public RWMemoryMember {
+class RWExit: public BitAccessibleRegister {
public:
RWExit(TraceValueRegister *registry, const std::string &tracename="");
protected:
@@ -84,7 +84,7 @@ class RWExit: public RWMemoryMember {
//! abort() on access memory
/*! Any access to this memory will instantly stop simulavr. */
-class RWAbort: public RWMemoryMember {
+class RWAbort: public BitAccessibleRegister {
public:
RWAbort(TraceValueRegister *registry, const std::string &tracename="");
protected:
--
2.1.4
_______________________________________________
Simulavr-devel mailing list
Simulavr-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/simulavr-devel
