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

[Klaus Rudolph]

Hi,

I personally dislike any solution which needs macros, cast-operators and other 
things. I believe that it is ever possible to write code without such kind of 
coding.

The next thing is that I believe to add any kind of run time switch for special 
kind of avr instruction behavior change slows again down the simulation. It is 
slowed down over the years a lot.

I would prefer a solution which generates the executing object in instruction 
decoding phase while reading flash. So there is no need to redo every decoding 
while executing. For SBI instruction everything is known in decoding stage, so 
I see no need to do anything in the execution phase of the instruction.

And yes, I see no need to replace CRTP to static callbacks. I would not apply 
any patch which moves in this direction.



Regards
 Klaus




> Gesendet: Freitag, 30. Juni 2017 um 01:06 Uhr
> Von: panic <address@hidden>
> An: address@hidden
> Betreff: Re: [Simulavr-devel] [PATCH] use static callbacks instead of 
> template param for IOReg
>
> ping?
> I would /really/ like to see some progress here....
> 
> - panic
> 
> panic:
> > ping? any comments?
> > 
> > - panic
> > 
> > panic:
> >> Michael Hennebry:
> >>> Would something like this help:
> >> [snip]
> >>
> >> To me this looks a bit like reinventing std::function/std::bind.
> >>
> >> But I'd do something similar to your proposal, using std::function that
> >> was added in C++11. GCC in current Debian stable is 6.3.0. Since GCC/g++
> >> 6, -std= defaults to c++14, so the feature is available for free:
> >>
> >> http://en.cppreference.com/w/cpp/utility/functional
> >>
> >>
> >> Step 1:
> >> ------
> >> In IOReg class remove the template parameter and change the
> >> getter_t/setter_t to:
> >>
> >>   typedef std::function<unsigned char(void)> getter_t;
> >>   typedef std::function<void(unsigned char)> setter_t;
> >>
> >>
> >> Step 2:
> >> ------
> >> Peripherals then need to be updated like this:
> >>     ddr_reg(this, "DDR",
> >> -           this, &HWPort::GetDdr, &HWPort::SetDdr),
> >> +           std::bind(&HWPort::GetDdr, this),
> >> +           std::bind(&HWPort::SetDdr, this, std::placeholders::_1))
> >>
> >> Up to here, I've already implemented it. Testsuite runs through mostly
> >> fine*. A patch is attached.
> >>
> >> Step 3:
> >> ------
> >> Add a set_bit(unsigned char bitpos, bool val) to IOReg, provide RMW
> >> default implementation if no specific callback for Set_bit was given.
> >> Then fix the insn decoder in AvrDevice.
> >>
> >>
> >> Discussion:
> >> ----------
> >> The syntax could be simpified if the IOReg itself (or RWMemoryMember)
> >> handled their unsigned char value themselves, instead of being a wrapper
> >> to get/set callbacks. We could then change the implementation of IOReg
> >> to just trigger "OnWrite" or "OnRead" callbacks that don't need any
> >> parameters, thus, we could omit the std::placeholders::_1.
> >> The IOReg/RWMemoryMember would then also make sure that the tracers are
> >> updated, which should avoid bugs related to wrong/forgotten tracing as
> >> it already happend multiple times.
> >>
> >>
> >> Cheers,
> >> panic
> >>
> >>
> >> *:
> >> I'm getting a testsuite error with the normal HEAD that also appears
> >> with my patch. To me, the test case seems to have a one-off issue. When
> >> I add round() to the testcase, they run fine:
> >>
> >>> ----------------------- regress/modtest/adc_diff_t25.py 
> >>> -----------------------
> >>> @@@ -46,7 -46,7 +46,7 @@@ class TestCase(SimTestCase)
> >>>      else:
> >>>        rng = 512
> >>>      v = self.sim.getWordByName(self.dev, "adc_value")
> >>> -    e = int(((pValue - nValue) / refValue) * rng) & 0x3ff
> >>> +    e = int(round((pValue - nValue) / refValue)) * rng) & 0x3ff
> >>>      self.assertEqual(v, e, "expected adc value is 0x%x, got 0x%x" % (e, 
> >>> v))
> >>>  
> >>>    def test_00(self):
> >>
> >> Since the upgrade to GCC 6, I see another testsuite error (already
> >> occurs when run on HEAD, independent of my changes):
> >>
> >>> ======================================================================
> >>> FAIL: test_00 (eeprom.TestCase)
> >>> eeprom_atmega16::check read and write eeprom data
> >>> ----------------------------------------------------------------------
> >>> Traceback (most recent call last):
> >>>   File "eeprom.py", line 70, in test_00
> >>>     self.assertValue(0x66)
> >>>   File "eeprom.py", line 19, in assertValue
> >>>     self.assertComplete()
> >>>   File "eeprom.py", line 16, in assertComplete
> >>>     self.assertEqual(c, 1, "function isn't complete (complete=%d)" % c)
> >>> AssertionError: function isn't complete (complete=2)
> >>>
> >>> ======================================================================
> >>> FAIL: test_00 (eeprom.TestCase)
> >>> eeprom_atmega128::check read and write eeprom data
> >>> ----------------------------------------------------------------------
> >>> Traceback (most recent call last):
> >>>   File "eeprom.py", line 54, in test_00
> >>>     self.assertValue(0x33)
> >>>   File "eeprom.py", line 19, in assertValue
> >>>     self.assertComplete()
> >>>   File "eeprom.py", line 16, in assertComplete
> >>>     self.assertEqual(c, 1, "function isn't complete (complete=%d)" % c)
> >>> AssertionError: function isn't complete (complete=2)
> >>>
> >>> ----------------------------------------------------------------------
> >>>
> >>>
> >>> _______________________________________________
> >>> Simulavr-devel mailing list
> >>> address@hidden
> >>> https://lists.nongnu.org/mailman/listinfo/simulavr-devel
> > 
> > 
> > _______________________________________________
> > Simulavr-devel mailing list
> > address@hidden
> > https://lists.nongnu.org/mailman/listinfo/simulavr-devel
> > 
> 
> 
> _______________________________________________
> Simulavr-devel mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/simulavr-devel
>