repetier / Repetier-Firmware

Firmware for Arduino based RepRap 3D printer.
815 stars 734 forks source link

Display controller hangs after USB is disconnected. #1070

Closed nukem closed 3 years ago

nukem commented 3 years ago

The display controller hangs, no button response after disconnecting usb connection to repetier host or any serial controller. The display responds again when the serial connection is re-established. Printing is not affected just the display.

Any clue on what part of the code should I check? version current dev2 controller due display controller Display20x4

Thanks.

nukem commented 3 years ago

Memory usage on compile:

Building .pio\build\digix\firmware.bin
Advanced Memory Usage is available via "PlatformIO Home > Project Inspect"
RAM:   [==        ]  19.0% (used 18648 bytes from 98304 bytes)
Flash: [===       ]  31.7% (used 166104 bytes from 524288 bytes)
Configuring upload protocol...
AVAILABLE: atmel-ice, blackmagic, jlink, sam-ba, stlink
CURRENT: upload_protocol = sam-ba
repetier commented 3 years ago

Just tested it on 2 printers with dev2 but controller was still responsive. Only difference was that it was graphic controller. But buttons are not part of display and handled in handleKeypress in GUI.cpp. It gets called around 4000 times per second from pwm interrupt and also in GUI::update. Does the display still update e.g. temperatures so it gets refreshed? Display is updated in this function that is called by Commands::checkForPeriodicalActions. This function must be called frequently and if it does not happen that would be a problem. That is why all slow functions like wait for end of moves call it while waiting. If that stops being done it is a problem.

But I do not see a way it would happen as you describe. Base loop is Commands::commandLoop where also serial is read. But same functions also reads from sd card so if print still works it is not blocked and the function is working.

Does this always happen or randomly? What is when you start without serial? Does it work then? Question is if serial was receiving when you stopped would this make it stick somehow until this started receiving command is finished. On idle printers it is hard to disconnect exactly then so it could be good luck that I had no problems.

nukem commented 3 years ago

I disconnect using the GUI on repetier host. I also tried on just a normal serial terminal.

The temperature or XY position is not updated on the screen after disconnect. It gets updated when you connect again.

After disconnect you can press Back or Ok button but the loop will just update the gui one level after you connect the serial the screen shows the one level up or down depending on which one you press. So it still responds to button events after disconnect but it will just do one level of Back or Ok after pressing them multiple times.

It is very consistent not random. It has no issues using the printer stand alone without serial or using the printer with the serial connected all the time. The serial terminal is not sending anything and the behavior is the same.

Thanks for the reply, I'll investigate some more since my usage will be connecting and disconnecting to the printer, I didn't notice it because I was not disconnecting before.

nukem commented 3 years ago

I just tried this:

Start printer, connect Repetier Host and pull the USB cord without disconnecting using the gui and the screen doesn't hang. Same response using a serial terminal.

My board is using USBSerial.

nukem commented 3 years ago

I tried this:

void GUI::update() { TOGGLE_VAR(UI_BACKLIGHT_PIN);

Disconnecting the serial stops the backliight from blinking, so gui::update is not called anymore when disconnecting. The printing movement also stops if I wait long enough. Pulling the USB cable while connected has no effect the backlight still blinks.

repetier commented 3 years ago

Since you mentioned Repetier-Host I tried to reproduce it with my due based board on Serial and rumba32 with server and both worked as expected. I guess you mean SerialUSB now USBSerial?

The problem is checkForPeriodicalActions does not get called any more or executePeriodical never gets true so it moves to the GUI::update at the bottom of the function. This value gets set in pwm timer.

But when light blinks

undef IO_TARGET

define IO_TARGET IO_TARGET_100MS

include "../io/redefine.h"

gets called and that is behind if (!executePeriodical) { return; // gets true every 100ms }

and after that there is no return so update should be called. So with that logic we have to assume it gets called. In update this is where display gets updated:

if (timeDiff < 60000 && (timeDiff > 1000 || contentChanged)) { // Com::printFLN(PSTR("upd:"), (int32_t)timeDiff); refresh(); lastRefresh = HAL::timeInMilliseconds(); contentChanged = false; }

keys work as you say. That is condition contentChanged. For regular update it would be timeDiff > 1000 to get an update every second. So that is failing.

Can you add that with if (timeDiff < 60000 && (timeDiff > 1000 || contentChanged)) { // Com::printFLN(PSTR("upd:"), (int32_t)timeDiff); refresh(); lastRefresh = HAL::timeInMilliseconds(); contentChanged = false; } else if (timeDiff > 2000) { lastRefresh = HAL::timeInMilliseconds(); }

if somehow timeDiff gets > 60000 it would stop updating. No idea how that would happen by disconnecting, but that would reset the value. Only thing I might think of is that closing keeps in serial get until a timeout which is higher than 60 seconds so that rule triggers and it is the stuck. Would also mean directly after closing connection there will be no reaction to buttons.

repetier commented 3 years ago

uups. Missed that you used toggle in update function and not with M355. But if it does not get called how does it update with keys pressed? It only gets called form that one function.

How did you print? From sd card or serial? If it stops moves after buffers are empty it means interrupts work maybe because of higher priority to usb interrupt. Once interrupt for usb serial hangs only higher priority interrupts run and it does not come back to main thread. That would be bad and should trigger watchdog. Do you have watchdog enabled?

nukem commented 3 years ago

I mean it only does one more loop on the gui codes after disconnecting so only one keypress is processed in the loop so only one level of menu change up or down then it's stuck. So I think it got something to do with serial.

USBSerial the one used by SAM3 for dfu.

I didn't have watchdog on. After enabling it I get the message Reset by watchdog.

repetier commented 3 years ago

Ok, so that looks like it is blocking in usb serial event handling. When you unplug the reason for blocking is not present. I wonder what would cause this. I mean host is not communicating any more just like terminal when it is closed (or did that not happen with terminal?). Unfortunately I have no idea how that works. Was always happy that Arduino libraries provided this code part:-)

nukem commented 3 years ago

Thanks for the code loop overview I don't need to read most of them now :-)
It's the same behavior on terminal if you switch com ports, but unplugging the wire doesn't affect the code.

My Davinci 2.0A Duo is printing great I just notice this behavior when I was working on timing out the backlight of the controller. I returned everything to dev2 HEAD thinking I messed something up.

The code architecture is amazing by the way! I was trying to do this:

IO_OUTPUT(controllerBackLightPin, UI_BACKLIGHT_PIN)
LIGHT_STATE_MONOCHROME(controllerBackLightState)
LIGHT_COND(controllerBackLightState, true, !GUI::isGUIIdle(), 255, 255, 255, 255)
LIGHT_SOURCE_MONOCHROME(controllerBackLightDriver, controllerBackLightPin, controllerBackLightState)

I added the GUI::isGUIIdle() then I encountered this behavior.

Is there a function a can check to see if the GUI is idle so I can map the backlight to it?

repetier commented 3 years ago

Gui is never busy, so not sure what you mean with gui idle. You might check if there are moves buffered to check if printer is busy or if you are in print mode. Printer::isPrinting() would then be a flag you could use. Especially on sd print and printing using our server this is true. It would also distinguish manual moves and real prints.

And thanks for liking the architecture. I know it is a bit more complex to configure but with the modules it is also much more flexible. Like a block building system for printer.

nukem commented 3 years ago

It checks the last interaction (button clicks) on the controller so it can turn off the backlight of the controller using the awesome define class generator.

My GUI::isGUIIdle() is working I just met this disconnect bug.

It was easy to configure I was able to use my Davinci 2.0 without much coding, I just added an option to have the ui_keys again because my controller does not have an encoder just buttons. It uses 16x4 lcd but the 20x4 driver is working fine without coding it just scrolls the long messages.

nukem commented 3 years ago

I have narrowed down the code where it hangs and triggers watchdog.

void Commands::checkForPeriodicalActions(bool allowNewMoves) {
..........
    if (Printer::isAutoreportTemp()) {
        millis_t now = HAL::timeInMilliseconds();
        if (now - Printer::lastTempReport > Printer::autoTempReportPeriodMS) {
            Printer::lastTempReport = now;
            Com::writeToAll = true; // need to be sure to receive correct receipient
            Commands::printTemperatures();
        }
    }
nukem commented 3 years ago

I found this patch for the Arduino USBCore.cpp to fix my issue.

File: USBCore.cpp
line: 182
//  Blocking Send of data to an endpoint
uint32_t USBD_Send(uint32_t ep, const void* d, uint32_t len)
{
    uint32_t n;
    int r = len;
    const uint8_t* data = (const uint8_t*)d;

    if (!_usbConfiguration)
    {
        TRACE_CORE(printf("pb conf\n\r");)
        return -1;
    }

    while (len)
    {
        if(ep==0) n = EP0_SIZE;
        else n =  EPX_SIZE;
        if (n > len)
            n = len;
        len -= n;
//USB PATCH to avoid dead loop
        int count=0;
    while( UOTGHS_DEVEPTISR_TXINI != (UOTGHS->UOTGHS_DEVEPTISR[ep & 0xF] & UOTGHS_DEVEPTISR_TXINI ))
        {
            count++;
            if (count>10000) return len;
        }
        UDD_Send(ep & 0xF, data, n);
        data += n;
    }
    //TXLED1;                   // light the TX LED
    //TxLEDPulse = TX_RX_LED_PULSE_MS;
    return r;
}

USB serial write communications don't hang anymore!

repetier commented 3 years ago

Watchdog triggers independent of position. It needs pwm interrupt to be run AND periodicalActions to be executed. This is prevented by serial.

But with periodical writing temperatures that is in deed a likely place. When the serial does not get closed so firmware side thinks it is still listening the output buffer gets full and sooner or later it then needs to wait for it to empty so we can continue with execution. But the serial output is blocking because at host side no one polls the usb data (because it is closed).

repetier commented 3 years ago

Ok exactly as I expected. Only thing is your solution is sub optimal. While it does not block any more it still means that when it happens you are wasting 10000 loops on every try. So cpu load increases heavily.

What we need is that the serial starts ignoring any sends. This rule is applied when usb cable is plugged. And it does not happen when connection is only closed which is the source of the problem.

nukem commented 3 years ago

As you mentioned arduino code is sketchy, their USB write loops forever on UDD_Send() because of not enough testing.

Well it was a learning experience now I know about the following files of repetier and how convenient to expand it: events.h

I don't know what you will do with it, my printer is now ok and will probably be reliable even if whatever is connected on the usbserial get disconnected.

Maybe the check can be done on the repetier side of the code so normal users don't need to fix the USBCore.cpp

repetier commented 3 years ago

I will check a bit further as time allows. In CDC.cpp you see

size_t Serial_::write(const uint8_t *buffer, size_t size)
{
    /* only try to send bytes if the high-level CDC connection itself
     is open (not just the pipe) - the OS should set lineState when the port
     is opened and clear lineState when the port is closed.
     bytes sent before the user opens the connection or after
     the connection is closed are lost - just like with a UART. */

    // TODO - ZE - check behavior on different OSes and test what happens if an
    // open connection isn't broken cleanly (cable is yanked out, host dies
    // or locks up, or host virtual serial port hangs)
    if (_usbLineInfo.lineState > 0)
    {
        int r = USBD_Send(CDC_TX, buffer, size);

So the main problem is _usbLineInfo.lineState > 0 should be 0.

repetier commented 3 years ago

Ok I have added a new usb serial implementation based on atmel usb. That one is working without this issue plus it gets no missed usb data due to hardware flow control which the due implementation also did not have. So think that issue is now solved.