bugSimulavr: an AVR simulator - Bugs: bug #40586, example/feedback does not work

 
 

bug #40586: example/feedback does not work

Submitter:  Markus Hitter <hitterm>
Submitted:  Fri 15 Nov 2013 01:18:37 AM UTC
   
 
Category:  Simulation Severity:  3 - Normal
Item Group:  None Status:  Wont Fix
Privacy:  Public Assigned to:  tomk
Open/Closed:  Closed Component Version:  * simulavr
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 17 Nov 2013 05:38:19 PM UTC, comment #8: 


> Best way to run this example is to type "make do" in
> examples/feedback directory. This needs, of course, that
> TCL ist configured (and installed) and also iTCL is installed.


Yes, this is what I slowly learn, too. These examples actually work, but it's not documented and even less intuitive. Intuitiviness is very imprtant these days, in the RepRap projects I maintain people run away if they don't see some results within the first 10 minutes of trying. You can add build times on top of these 10 minutes, but not expect the user to read before trying.

For example, if TCL isn't configured, typing "make" could bail out with something like: "Built the AVR binary, but you won't be able to run it, because TCL isn't configured. Try to reconfigure and rebuild simulavr with passing --enable-tcl to configure."

Another question is, why the iTCL dependency for such a simple piece of code? :-)

Markus Hitter <hitterm>
Sun 17 Nov 2013 04:20:34 PM UTC, comment #7: 

Hi Markus,

the problem with examples/feedback is, that (not only) register 0x20 is used as a special debug register, not as normal PINF register. But this needs also to give the commandline options for that, if you want to run this example, see Makefile.am.

Best way to run this example is to type "make do" in examples/feedback directory. This needs, of course, that TCL ist configured (and installed) and also iTCL is installed.

I'll push in the next days your hint to extend README.

Thomas K. <tomk>
Group administrator
Sat 16 Nov 2013 09:30:27 AM UTC, comment #6: 

I got portToggleFeature wrong.
'Tis a HWPort member.  HWPort's constructor defaults it to false.
There is no explicit mention of it in any atMega128 code.
That is correct for an atMega128.
From the source, feedback does not appear to do much of anything.
From the names in debugio.c it uses location 0x20 as an output location.
That would not work on a real atMega128. 0x20 refers to PINF.
feedback itself appears to use no input.

I noticed that portSize, portMask and portToggleFeature are separate non-const members or HWPort.
At the least, they should be const.
I think that they should be implicit in the type,
which means that HWPort should be a class template.
Also, portMask is calculated from portSize.
The assumption seems to be that the high pins will be the missing pins.
Do we want to rely on that?
If so, it really shold be documented.

When making HWPort into a class template,
mosts of the work could be handled by a base class:
class HWPortBase {
public:
    HWPortBase::HWPortBase(AvrDevice *core, const string &name);
    void finish(int tt, int mask) ;
    void Reset(void) CalcOutputs();
    Pin& GetPin(unsigned char pinNo);
    virtual void CalcPin(void) = 0;
    void CalcOutputs(void);
    virtual string GetPortString(void) = 0;
    virtual void SetPin(unsigned char val);
    virtual ~HWportBase() {}
} ;

// 45678 1 2345678 2 2345678 3 2345678 4 2345678 5 2345678 6 2345678 7 2345678 8

template<unsigned char portMask, bool portToggleFeature>
class HWPort : public HWPortBase {
public:
    HWPort(AvrDevice *core, const std::string &name) : HWPortBase(core, name)
    {
        for(unsigned char tt=7, mask=0x80; mask; --tt, mask>>=1) {
            if(mask & portMask) finish(tt, mask);
        } // tt, mask
    } constructor

    virtual void CalcPin(void)
    {
        // calculating the value for register "pin" from the Pin p[] array
        pin = 0;
        for(unsigned tt = 7, mask=0x80; mask; --tt, mask>>=1) {
            if((mask & portMask) && p[tt].CalcPin()) pin |= mask;
        } // tt, mask
    } // CalcPin

    virtual void HWPort::SetPin(unsigned char val) {
        if(portToggleFeature) {
            port ^= val;
            CalcOutputs();
            port_reg.hardwareChange(port);
        } else {
            avr_warning("Writing of 'PORT%s.PIN' (with %d) is not supported.",
                        myName.c_str(), val );
        }
    } // SetPin

    virtual HWPort::~HWPort() {
        for(unsigned char tt=7, mask=0x80; mask; --tt, mask>>=1) {
            if(mask & portMask) {
                UnregisterTraceValue(pintrace[tt]);
                delete pintrace[tt];
            }
        } // tt, mask
    } // destructor
} ;

Michael Hennebry <skeeve2>
Sat 16 Nov 2013 06:20:03 AM UTC, comment #5: 

The toggle PORT with PIN flag is probably in atmega128.h .
The flag likely gets passed to a HWPort constructor.
In any case, on an atMega128, assignments to PIN registers should fail.

Michael Hennebry <skeeve2>
Fri 15 Nov 2013 11:32:44 PM UTC, comment #4: 

Well, code in atmega128.cpp isn't involved here, other than by defining seven I/O ports. The next step back in the backtrace (see below) is rwmem.h, which is a C++ template and as such beyond my C++ skills. I can only guess how the pieces fall together. The code tries to set single pins to values lie 'A' or 'F' which somehow makes me feel like there's something seriously confused.

The backtrace:
Breakpoint 1, HWPort::SetPin (this=0x15cd6f0, val=68 'D') at hwport.cpp:159
159 void HWPort::SetPin(unsigned char val) {
(gdb) bt
#0  HWPort::SetPin (this=0x15cd6f0, val=68 'D') at hwport.cpp:159
#1  0x000000000045533d in IOReg<HWPort>::set (this=<optimized out>, val=<optimized out>) at rwmem.h:273
#2  0x0000000000478fa6 in RWMemoryMember::operator= (this=0x15cda48, val=68 'D') at rwmem.cpp:69
#3  0x00000000004332d4 in AvrDevice::SetIOReg (this=<optimized out>, addr=<optimized out>, val=<optimized out>)
    at avrdevice.cpp:451
#4  0x000000000043a6c1 in avr_op_OUT::operator() (this=0x6c37e0) at decoder.cpp:1178
#5  0x0000000000433e9f in AvrDevice::Step (this=0x6bc790, untilCoreStepFinished=@0x7fffffffd9ef: false,
    nextStepIn_ns=0x7fffffffd998) at avrdevice.cpp:331
#6  0x000000000048071f in SystemClock::Step (this=this@entry=0x6b9420 <SystemClock::Instance()::obj>,
    untilCoreStepFinished=@0x7fffffffd9ef: false) at systemclock.cpp:154
#7  0x0000000000480910 in SystemClock::Endless (this=0x6b9420 <SystemClock::Instance()::obj>) at systemclock.cpp:217
#8  0x0000000000406a42 in main (argc=<optimized out>, argv=<optimized out>) at cmd/main.cpp:454

Markus Hitter <hitterm>
Fri 15 Nov 2013 11:07:33 PM UTC, comment #3: 

For ATmega128's and similar vintage devices, false is the correct value.
Whether there should be a default value is another issue.
I would say that there should not be.
Regardles of that issue,
coding the PIN toggles PORT feature into the simulated ATmega128 is a wrong thing to do.

Michael Hennebry <skeeve2>
Fri 15 Nov 2013 10:26:17 PM UTC, comment #2: 

Thanks for the insight. Does this now mean all the "older" devices shoud change the default value or is it right and the problem causing all the warnings is elsewhere?

Markus Hitter <hitterm>
Fri 15 Nov 2013 09:28:07 PM UTC, comment #1: 

In a newer 8-bit AVR,
PINF=0x0F would toggle the lower nybble of PORTF.
ATmega128's are not newer 8-bit AVR's.

Michael Hennebry <skeeve2>
Fri 15 Nov 2013 01:18:37 AM UTC, original submission:  

Trying to get a grip on how to use SimulAVR, I tried with the "feedback" example. First of all, please find two patches attached, which (0001-*) simplifies the Makefile and (0002-*) adds instructions on how to run the example. Well, the latter is what I found out so far, at least something visible happens.

Running the simulator with the given firmware gives me no less than 910 lines similar to these:

$ ../../src/simulavr -d atmega128 -f feedback.elf
WARNING: file hwport.cpp: line 165: Writing of 'PORTF.PIN' (with 101) is not supported.
WARNING: file hwport.cpp: line 165: Writing of 'PORTF.PIN' (with 113) is not supported.
WARNING: file hwport.cpp: line 165: Writing of 'PORTF.PIN' (with 117) is not supported.
[...]

I tracked this down to commit 3f6a6882065 (Dec 2009), which introduced a "PIN toggle PORT". About all chips, except the ATtiny2313, ignore this feature / use the default value, which means false. Setting the default to true in src/hwport.cpp makes all the warnings go away.

To be honest, I have no idea what "toggle port" means and why the default apparently forbids to use a port. Given the numbers which are attempted to be written, "feedback" tries to write out it's texts here.

Regardless of the default here, "feedback" doesn't do anything close to what the README says. A connection is reported to be established, any keypress other than a plain return results in an exception:

$ telnet localhost 7777
Trying 127.0.0.1...
New Connection on Simulation Feedback Server established!
Socket: sock6  from Address 127.0.0.1  and port 46995
Connected to localhost.
Escape character is '^]'.
adc0 0 adc1 0
__ack X     <== hitting Enter here
__ack X 2
invalid command name "2"
    while executing
"2"
    ("eval" body line 1)
    invoked from within
"eval $x"
    (object "::guiMaster_sock6" method "::Master::ReadFromSimulavr" body line 34)
    invoked from within
"::guiMaster_sock6 ReadFromSimulavr"

After such an exeception, the connection is entirely unresponsive.

Markus Hitter <hitterm>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by tomk (Posted a comment)
  • -email is unavailable- added by skeeve2 (Posted a comment)
  • -email is unavailable- added by hitterm (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2013-11-17 tomk StatusNone Wont Fix
        Assigned toNone tomk
        Open/ClosedOpen Closed
    2013-11-15 hitterm Attached File- Added 0001-example-feedback-no-python-involved-here.patch, #29622
        Attached File- Added 0002-example-feedback-tell-our-fellow-users-on-how-to-run.patch, #29623

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code