Get UBL's G29 P1 (Automated Probing) working again.

[Roxy-3D]

Incorrect optimizations of data types and ternary operators caused some
issues. All fixed now...

[thinkyhead]

What does this mean?

// We can't do lcd_setstatuspgm() without having it continue;

[Roxy-3D]

What does this mean?

I don't know the full answer. This is something that needs to be better understood. But trying to use lcd_setstatuspgm() to output a message on the LCD Panel some how causes UBL to think the Encoder Wheel has been clicked. And as a result of calling lcd_setstatuspgm(), the UBL logic thinks it should continue....

So... Just to get the G29 P1 automated probing going (and the G26 Mesh Validation Pattern also), I'm not doing any lcd_setstatuspgm() calls until this is better understood.

[thinkyhead]

Ok. I will investigate the cause by observing the code, and then fix it.

[thinkyhead]

So… Is it your observation that using lcd_setstatuspgm causes ubl_lcd_clicked to subsequently return true? I'm dubious about that, because ubl_lcd_clicked directly checks the pin. There's no reason that the hardware pin state should be affected. The only side-effect of lcd_setstatuspgm is that lcdDrawUpdate is set to LCDVIEW_CLEAR_CALL_REDRAW.

Note that if you call lcd_setstatuspgm with a "level" of 99, it will prevent later calls to lcd_setstatuspgm from working until you have called lcd_reset_alert_level.

[Roxy-3D]

The only side-effect of lcd_setstatuspgm is that lcdDrawUpdate is set to LCDVIEW_CLEAR_CALL_REDRAW.

Yes... It maybe this is at the root of the problem. I should check and see if that is causing it.

Note that if you call lcd_setstatuspgm with a "level" of 99, it will prevent later calls to lcd_setstatuspgm from working until you have called lcd_reset_alert_level.

Yes... The 99 was just to make sure no other routine declared itself as 'more important'. But I never got to the point of having to use the lcd_reset_alert_level(). After calling the lcd_setstatuspgm() the code thought the Encoder Wheel was clicked and continued.

[Roxy-3D]

Also... Here is something else that is interesting... This logic is going to break once we understand and change what is going on inside lcd_setstatuspgm(). But I did end up getting lcd_setstatuspgm() to work in G26_Mesh_Validation_Tool.cpp. But... It most certainly is not how we want it to be. By calling it multiple times and letting it discard a message... I can get it to do what is needed:

    do {
      if (ubl_lcd_clicked()) {              // Check if the user wants to stop the Mesh Validation
        #if ENABLED(ULTRA_LCD)
          lcd_setstatuspgm(PSTR("Mesh Validation Stopped."), (uint8_t) 99);
          lcd_quick_feedback();
        #endif
        while (!ubl_lcd_clicked()) {         // Wait until the user is done pressing the
          idle();                            // Encoder Wheel if that is why we are leaving
          lcd_setstatuspgm(PSTR(" "), (uint8_t) 99);
        }
        while ( ubl_lcd_clicked()) {         // Wait until the user is done pressing the
          idle();                            // Encoder Wheel if that is why we are leaving
          lcd_setstatuspgm(PSTR("Unpress Wheel "), (uint8_t) 99);
        }
        goto LEAVE;
      }

(I fully understand this needs to be re-done. I was just trying to get G29 P1 and G26 working again. And probably... the #endif should be moved down to enclose all of the lcd_setstatuspgm() calls.)

[thinkyhead]

- lcd_setstatuspgm(PSTR(" "), (uint8_t) 99);
+ lcd_reset_alert_level();
+ lcd_setstatuspgm(PSTR(""));

[Roxy-3D]

Oh!!!! OK! I'll try that in the morning. My printer is busy printing right now!

[thinkyhead]

Cool. Yep, lcd_reset_alert_level is needed to allow the alert message to work again. I have a set of changes, including that one, that I'll post as a PR which you can test tomorrow.

[thinkyhead]

The first patch is posted as #6152.

[Roxy-3D]

Also... Can you give me some higher level guidance about what you want me to do to UBL so that it handles things similar to M600. I might be able to start that work tomorrow.

[thinkyhead]

Let's take a look at a sample of M600 code:

do {
  // "Wait for filament extrude"
  lcd_filament_change_show_message(FILAMENT_CHANGE_MESSAGE_EXTRUDE);

  // Extrude filament to get into hotend
  destination[E_AXIS] += FILAMENT_CHANGE_EXTRUDE_LENGTH;
  RUNPLAN(FILAMENT_CHANGE_EXTRUDE_FEEDRATE);
  stepper.synchronize();

  // Show "Extrude More" / "Resume" menu and wait for reply
  KEEPALIVE_STATE(PAUSED_FOR_USER);
  wait_for_user = false;
  lcd_filament_change_show_message(FILAMENT_CHANGE_MESSAGE_OPTION);
  while (filament_change_menu_response == FILAMENT_CHANGE_RESPONSE_WAIT_FOR) idle(true);
  KEEPALIVE_STATE(IN_HANDLER);

  // Keep looping if "Extrude More" was selected
} while (filament_change_menu_response == FILAMENT_CHANGE_RESPONSE_EXTRUDE_MORE);

• First it calls a helper function to show a particular LCD screen, FILAMENT_CHANGE_MESSAGE_EXTRUDE. That's an enumerated value. The lcd_filament_change_show_message function just shows a screen based on the enum it receives. This is an alternative to exposing the names of the screen handler functions and the lcd_goto_screen function, and encapsulates these screens into a group. It's not a bad way to do it. The lcd_filament_change_show_message function takes care of any details like setting defer_return_to_status.

• Next it does a move, pulling filament into the hotend. This move is not interruptible, but it could be made so.

• Next we want to wait for the user to respond to an LCD menu. This is different from waiting for a click. We actually do want the user to interact with the menu, so we clear wait_for_user (in case it was set). Since M600 still sits in a loop, it's important to call KEEPALIVE_STATE(PAUSED_FOR_USER) for host keepalive. Here we show the FILAMENT_CHANGE_MESSAGE_OPTION screen, then wait until filament_change_menu_response no longer equalsFILAMENT_CHANGE_RESPONSE_WAIT_FOR. The menus linked from lcd_filament_change_option_menu will set this variable.

• Finally, the big loop will continue to run as long as the menu item selected is FILAMENT_CHANGE_RESPONSE_EXTRUDE_MORE.

This is more-or-less the cleanest way to interface with the LCD controller. Just let the LCD menu code set the current state of the UI, and have the gcode_Gnn function set and observe that state. If a global variable needs to be shown on the LCD, it might be brought in with extern where it's used, or custom values could be included in calls to your own lcd_ubl_show_message(my_enum, my_value) function….

You can see, for example, how some of the LCD screen handlers for M600 include the current hot-end temperature. Those values are just grabbed with accessors and continually displayed by the screen handler. In any case, the current menu screen will continue to run until it exits itself in response to a click or some other event. So, you can just put up an "Adjust Z" screen and wait for it to be exited with a click, for example. The code responding to the click could set a new state, such as "ubl_ui_state = UBL_UI_GOT_Z", and some observer (such as gcode_G29) could just sit around until that state arrives…. Sometimes the menus will manage changing screens, and sometimes the controlling/observing function will.

[Roxy-3D]

Wow! I only skimmed this because I'm watching a movie with friends and we have a few empty wine bottles sitting on the coffee table. But for sure, I'm going to re-read this in the morning before I start figuring out how to warm over the UBL code's interface to the LCD Panel.

Incidentally.... You might have noticed the mesh_edit() functions are right next to the babystep functions. I was trying to copy how they did things. But something is different and the numbers on the LCD Screen don't get updated if I just count on the ultralcd.cpp to call the function and update things. Right now... the G29 P4 has to call mesh_edit() to force the numbers to get updated. I tried copying what

    void _lcd_babystep_z() { _lcd_babystep(Z_AXIS, PSTR(MSG_BABYSTEPPING_Z)); }
    void lcd_babystep_z() { lcd_goto_screen(_lcd_babystep_z); babysteps_done = 0; defer_return_to_status = true; }

does, and it doesn't work when ubl_has_control_of_lcd_panel. If you can take a quick look at that and figure it out.... It would help me clean things up. Some how... my lcd_fine_tune_mesh() function doesn't keep getting called.

[ghost]

I'm watching a movie with friends and we have a few empty wine bottles sitting on the coffee table.

Don't you know that programmers aren't supposed to have a life? ;P

[Roxy-3D]

Don't you know that programmers aren't supposed to have a life? ;P

No... I didn't know that... But I do know not to try to write code when I've had a few drinks.

[thinkyhead]

it doesn't work when ubl_has_control_of_lcd_panel

Let's work on getting rid of that. The defer_return_to_status flag is all you need to take over the LCD and prevent the screen from changing unless properly triggered. Really study the MBL, M600, and PROBE_MANUALLY code (#6050) for a complete picture. Some notes…

• Screen handlers are only called if:
  • the encoder wheel is moved or the encoder button is pressed, or
  • if the lcdDrawUpdate state is set to one of the (non-zero) draw states

So, in order to make sure a screen handler keeps getting called, we have to set lcdDrawUpdate.

For example, for the hotend temperature in M600 we just set lcdDrawUpdate = LCDVIEW_KEEP_REDRAWING inside of the HOTEND_STATUS_ITEM() menu item. This causes the screen handler to be called again, but with lcdDrawUpdate set to zero (on character displays). Screen handlers check lcdDrawUpdate for a non-zero value before redrawing their content. The screens that display the hotend temperature also check this flag, but only for their editable value. The hotend temperature is updated unconditionally on every call to the screen handler.

As for lcd_babystep(), it only ever has to be called in response to a controller event. It is not called otherwise. So there's no funky setting of lcdDrawUpdate in that function. The _lcd_babystep function only sets lcdDrawUpdate = LCDVIEW_REDRAW_NOW in response to the controller wheel for the benefit of the last bit of logic. (LCDVIEW_REDRAW_NOW doesn't cause the screen handler to be called again.)

As for your mesh_edit functions, they need a re-work. You shouldn't be doing something like new_z = lcd_mesh_edit() calling an LCD handler directly. The correct way to do this would be to — sure, call something in ultralcd.cpp to put up the right screen — but use a state flag to wait for the value to become available, as M600 does. For example, you might have a bool named lcd_got_ubl_z that your screen handler sets when the button is clicked. And in your fine_tune_mesh function you would do something like:

lcd_fine_tune_mesh(); // which will clear lcd_got_ubl_z
while (!lcd_got_ubl_z) idle(); // _lcd_mesh_fine_tune will set this on click
new_z = mesh_edit_value;

Alternatively, the wait-for-mesh-value loop can be in ultralcd.cpp like so:

    float lcd_mesh_edit() {
      lcd_goto_screen(_lcd_mesh_edit);
      while (currentScreen == _lcd_mesh_edit) idle();
      return mesh_edit_value;
    }

Then you have _lcd_mesh_fine_tune respond to a click with lcd_goto_screen(...).

In any case, all controller event handling should be in ultralcd.cpp.

Capisce?