grblHAL / core

grblHAL core code and master Wiki
Other
305 stars 74 forks source link

Weird "ok" respond behavior #443

Closed nickshl closed 4 months ago

nickshl commented 4 months ago

I found that "ok" respond have weird behavior.

First, when I send command G38.2X100F10, grblHAL start moving axis until probe touched anything and then it reports probe position and "ok":

G38.2X100F10
[PRB:0.599,0.000,0.000:1]
ok

So far so good. When I real-time status report requested by sending "?" grblHAL respond with this:

<Idle|MPos:1.690,0.000,0.000|Bf:100,1024|FS:0,0>
ok

It seems to me that "ok" absolutely unnecessary there. <> by itself enough to figure out that respond to status request received. Additional "ok" can cause confusion if it respond to status request or to command. But, ok, it is how it is, no one will change that because of backward compatibility, which is fine. It easy enough to track if status report request was send, if <> response to it received, so "ok" should be ignored, it is not respond to command send earlier, right?

But it is not how it works. This is what happens when I send G38.2X100F10 command and multiple ? status requests while it executing:

G38.2X100F10
?
<Run|MPos:1.121,0.000,0.000|Bf:99,1024|FS:10,0|WCO:10.000,12.000,52.796>
?
<Run|MPos:1.289,0.000,0.000|Bf:99,1023|FS:10,0|Ov:100,100,100>
?
<Run|MPos:1.450,0.000,0.000|Bf:99,1022|FS:10,0>
?
<Run|MPos:1.598,0.000,0.000|Bf:99,1021|FS:10,0>
[PRB:1.678,0.000,0.000:1]
ok
ok
ok
ok
ok

It sends all ok after command is executed. It looks like now I have to track how many status reports request I send, how many responds I received and how many next "ok" should be ignore before I could count it as valid response to a command. Looks like unnecessary complexity to me. Is there a reason for this behavior?

terjeio commented 4 months ago

This does not happen for me, do you have a plugin running that misbehaves?

nickshl commented 4 months ago

I don't think so. I rebuild FW using Web Builder(STM32F4xx, Devtronic CNC Controller, EEPROM 24x256, Spindle Sync and Log for tuning, everything else left by default) and it still happens. Have you use ioSender to check? ioSender doesn't show those "ok". I use PuTTY to send those commands.

By the way, I noticed that you added CMD_MPG_MODE_TOGGLE to the core which is great(probably can/should be removed from keypad plugin now)! I just realize that my "brilliant" idea to control MPG by TX line state has one "minor" flaw - it is impossible send Cycle Start/Hold/Stop while PC Sender in control. I assume those commands can be send from MPG regardless who is in control? Anything else available when PC Sender in control(like Feed and Speed adjustment)?

And yes, I would not be me if I will not have any complains :) To have CMD_MPG_MODE_TOGGLE is great, but to have explicit CMD_MPG_MODE_ON and CMD_MPG_MODE_OFF commands is even better. May be we can add those too?

terjeio commented 4 months ago

I tested with PuTTY... So it must be a plugin?

By the way, I noticed that you added CMD_MPG_MODE_TOGGLE to the core which is great(probably can/should be removed from keypad plugin now)!

No, since what I added to the core is support for claiming a stream that cannot be shared with the keypad plugin. It is enabled by setting MPG_ENABLE to 2 in _mymachine.h.

I assume those commands can be send from MPG regardless who is in control?

Correct.

Anything else available when PC Sender in control(like Feed and Speed adjustment)?

All realtime commands can be used from the MPG when not in control.

To have CMD_MPG_MODE_TOGGLE is great, but to have explicit CMD_MPG_MODE_ON and CMD_MPG_MODE_OFF commands is even better. May be we can add those too?

I am not keen to add those as any MPG should keep track of the state by checking on the MPG element in the realtime report. If the MPG want to get the state it can send the CMD_STATUS_REPORT_ALL (0x87) to get it. And I want to stay away from most of the top bit set characters in ISO8859-1 - limiting how many are available for future expansion.

nickshl commented 4 months ago

I tested with PuTTY... So it must be a plugin?

No, I figure it out - it is PyTTY. "Local line editing" was set to "forced on" in my configuration. As result PyTTY wait for enter key to send the line. And it also send it with CR(and may be LF) which caused grblHAL to send "ok" in response.

Thanks for help to figure it out!

nickshl commented 4 months ago

If the MPG want to get the state it can send the CMD_STATUS_REPORT_ALL (0x87) to get it.

State of MPG request, or state of MPG control? If it show state of MPG control, then MPG can't figure out if grblHAL not willing to grant control(MPG may guess by state, but it is a guess) or if CMD_MPG_MODE_TOGGLE command got lost. And because it is toggle command it can't be resend by this reason.

terjeio commented 4 months ago

A full status report is automatically sent out on a denied/granted state change. And the current state can be queried at all times with the CMD_STATUS_REPORT_ALL command. So I do not understand what your issue is?

E.g. in my MPG implementations I handle the request and state change feedback asynchronously. On connect I request the current state and set the MPG mode based on the feedback - the mode is indicated by a visual clue. A button is used to trigger sending the CMD_MPG_MODE_TOGGLE command, and again the reported state is used to update the mode and visual clue. If the visual clue does not change the request was denied. On a reset I also request the current state. My message parser, which you can use if you want to, generates an event on a state change which is then used to set the MPG mode (listening/full control).

nickshl commented 4 months ago

If the visual clue does not change the request was denied.

"denied" or "can't be processed right now, delayed"? For example if ioSender in control and we start streaming gcode, state is "run" and then MPG send CMD_MPG_MODE_TOGGLE command - what happens? grblHAL can't grant control to MPG in this case. Will it ignore toggle request? Or will it remember it and grant control as soon ioSender stop streaming gcode and state will switch to "idle"?

In other words if I send CMD_MPG_MODE_TOGGLE command, then send CMD_STATUS_REPORT_ALL command and in response I see MPG:0, can I send CMD_MPG_MODE_TOGGLE command to request control again? Or will second CMD_MPG_MODE_TOGGLE command cancel first request?

terjeio commented 4 months ago

denied = can't be granted now, try again later. So no queueing.

... if I send CMD_MPG_MODE_TOGGLE command, then send CMD_STATUS_REPORT_ALL

The status report will be output automatically, there is no need to request it.

nickshl commented 4 months ago

denied = can't be granted now, try again later. So no queueing.

Cool. That works really good. Now I able to Hold/Continue/Stop on MPG while ioSender streaming gcode. However I not able to start program stream using MPG. Is there a way to let sender know that Start button on controller/MPG was pressed? Also ioSender correctly respond to Hold/Run state by disable/enable corresponded buttons, but look like it ignore Idle state that happens after stop button on MPG was pressed - I still have to press "Stop" in sender to unlock Cycle Start button.

terjeio commented 4 months ago

Change this code to:

ISR_CODE bool ISR_FUNC(stream_mpg_check_enable)(char c)
{
    if(c == CMD_MPG_MODE_TOGGLE)
        protocol_enqueue_foreground_task(stream_mpg_set_mode, (void *)1);
    else {
        protocol_enqueue_realtime_command(c);
        if(settings.status_report.pin_state && (c == CMD_CYCLE_START || c == CMD_CYCLE_START_LEGACY))
            sys.report.cycle_start = state_get() == STATE_IDLE;
    }

    return true;
}

to forward cycle start to the sender. I'll add it in the next commit.

Stop handling needs changes in the sender, perhaps also in the controller so a bit more involved.

nickshl commented 4 months ago

to forward cycle start to the sender. I'll add it in the next commit.

Thanks! Could you also check this pull request: https://github.com/grblHAL/STM32F4xx/pull/159 I commented out HAS_BOARD_INIT in board map and added "mpg_mode": 2 into driver.json for "Devtronic CNC Controller". I also removed "keypad": 1, and "i2c_strobe": 1 - I assume it for I2C keypad plugin, but don't think it is necessary by default.