Fix/4493 - Defer M20 commands until initial capability report is done #4577

kForth · 2022-07-14T02:18:20Z

What does this PR do and why is it necessary?

This is a proposed fix for issue #4493.

This PR tracks the start and end of the first capability report and defers any M20 commands until it's done.

The first M20 command is queued before the response from M115 is processed, meaning long filenames are not shown in the file list until it's refreshed.

How was it tested? How can it be tested by the reviewer?

Connect to any printer and observe the terminal output
Verify no M20 command is sent before the capability report is done
Verify an M20 command is sent shortly after the capability report finishes
- with the L parameter if EXTENDED_M20 is supported

What are the relevant tickets if any?

This is a fix for issue #4493.

Screenshots (if appropriate)

Not really screenshots but close enough.

Terminal Output Before

  Connected to: VIRTUAL(read_timeout=10.0,write_timeout=10.0,options={'_config_version': 1, 'enabled': True}), starting monitor
  Send: N0 M110 N0*125
  Recv: start
  Send: N0 M110 N0*125
  Recv: Marlin: Virtual Marlin!
  Recv: \x80
  Recv: SD card ok
  Recv: ok
  Send: N0 M110 N0*125
  Changing monitoring state from "Connecting" to "Operational"
  Recv: ok
  Send: N0 M110 N0*125
  Recv: ok
  Send: N1 M115*39
  Recv: ok
  Send: N2 M20*19
  Recv: FIRMWARE_NAME:Virtual Marlin 1.0 PROTOCOL_VERSION:1.0
  Recv: Cap:AUTOREPORT_TEMP:1
  Recv: Cap:AUTOREPORT_SD_STATUS:1
  Recv: Cap:AUTOREPORT_POS:0
  Recv: Cap:EMERGENCY_PARSER:1
  Recv: Cap:EXTENDED_M20:1
  Recv: ok
  Send: M155 S2
  Recv: Begin file list
  Recv: End file list
  Recv: ok
  [...]
  Recv: ok
  [...]
  Recv: ok

Terminal Output After

  Connected to: VIRTUAL(read_timeout=10.0,write_timeout=10.0,options={'_config_version': 1, 'enabled': True}), starting monitor
  Send: N0 M110 N0*125
  Recv: start
  Send: N0 M110 N0*125
  Recv: Marlin: Virtual Marlin!
  Recv: \x80
  Recv: SD card ok
  Recv: ok
  Send: N0 M110 N0*125
  Changing monitoring state from "Connecting" to "Operational"
  Recv: ok
  Recv: ok
  Send: N0 M110 N0*125
  Recv: ok
  Send: N1 M115*39
  Recv: FIRMWARE_NAME:Virtual Marlin 1.0 PROTOCOL_VERSION:1.0
  Recv: Cap:AUTOREPORT_TEMP:1
  Recv: Cap:AUTOREPORT_SD_STATUS:1
  Recv: Cap:AUTOREPORT_POS:0
  Recv: Cap:EMERGENCY_PARSER:1
  Recv: Cap:EXTENDED_M20:1
  Recv: ok
  Send: M155 S2
  Recv: ok
  [...]
  Recv: ok
  Send: M20 L
  Recv: Begin file list
  Recv: End file list
  Recv: ok

Further notes

I'm unsure how likely/possible it is for the 'cap:' lines to be interrupted, but it could cause the M20 to be sent before the EXTENDED_M20 capability is reported.

An alternative solution is to refresh the list as soon as the EXTENDED_M20 capability is reported (like below), but that will double report the file list.

elif (
          capability == self.CAPABILITY_EXTENDED_M20 and enabled
      ):
          self._logger.info(
              "Firmware states that it supports long filenames"
          )
         self.refreshSdFiles()

Thoughts are appreciated.

kForth · 2022-07-16T14:30:22Z

I've realized another potential bug in this approach: the server will block all calls to refreshSdFiles if the printer firmware never reports any 'cap:' lines.

This may not be a big issue since the reprap wiki shows good support the M115 command, and all firmwares that support M20 also support M115.

An alternative approach is to force the first command to be M20 L, probably activated by a setting in the printer profile.

A few caveats with this approach, if the option is set to True:

First refresh will be M20 L even if printer responds with Cap:EXTENDED_M20:0
Every refresh will be M20 L if the printer never reports a Cap:EXTENDED_M20 line

There is no change to the existing behaviour if this option is set to False.

You can see my implementation of this on my fix/4493-assume-extended-m20-option branch .

Thoughts are appreciated.

cp2004 · 2022-07-17T18:01:03Z

I was about to test if no capabilities would result in an issue. If there's one thing that's certain, it's that you are always uncertain on what might happen with printer communication - random things break all the time. Creality 2x temp reporting for example, that's quite an unexpected break.

I like the idea of sending an M20 L command as soon as the capability report comes in - it's fairly robust. The issue then becomes when to send a regular M20, if the capability is not received.

With the assumption method you have there, I think the option would be more at home in the serial connection settings than the printer profile. Yes, I agree that it could change on a printer-by-printer basis, but none of the other serial settings are part of a profile - unless they are all migrated then I think it should stick with the rest of them. The only thing I am worried about is some firmware variants complaining about an 'Unknown parameter' - I have tested my printer (Marlin 2.0.8.2), it doesn't (the simulator crashed). One I would like to see is Prusa, since that has had some of the strangest protocol behaviour before (aside from Creality).

To summarise the options - all have one downside that I can see.

Original PR: Wait until capabilities are finished reporting to refresh files
- M20 may never be sent if no cap report received
Listing files when report received
- Duplicates the listing command
Assume LFN support, send M20 L unless configured/detected as not supported
- Will some printers not respond to this?

I don't like this kind of dilemma...

jneilliii · 2022-07-17T18:31:00Z

Assume LFN support, send M20 L unless configured/detected as not supported

Will some printers not respond to this?

I was thinking this would be the way to go, but that's under the assumption that the unknown parameter would just be ignored. Gina may have more insight when she gets back from vacation in regard to that. Pretty sure Marlin is pretty forgiving with stuff like that, but not sure on other firmwares.

jneilliii · 2022-07-17T18:45:45Z

Have reached out on Discord to some PrusaFirmware users to see what happens when M20 L is sent to their printer.

jneilliii · 2022-07-17T21:07:00Z

adding responses from PrusaFirmware.

M20.log

kForth · 2022-07-18T03:08:12Z

Thanks for the feedback.

Thinking more about this, I may prefer the 'Assume LFN Support' option because it doesn't modify the original command order or timing.

A fourth option is:

Add a setting to override the command used in refreshSdFiles.
- See my implementation on my fix/4493-sd-refresh-command-setting branch

Then the user could override it to always be M20 L or whatever command their firmware may prefer.

The only potential issue I see right now is how to handle adding the L parameter for commands other than M20.
The capability is EXTENDED_M20 so I would not add it to other commands, but I'm not sure the best solution.

I've also updated my original implementation so that there's a setting to enable the feature, default is disabled.
May want to add a warning about potentially breaking the sd card file list if the printer doesn't respond to M115 correctly.

jneilliii · 2022-07-18T04:19:21Z

I like both these as optional settings, deferring file list loading and what command to use to list sd files. After thinking about it though, the later would technically resolve both allowing the user to manually override the command being sent initially prior to the capability report being read/loaded.

jneilliii · 2022-07-18T04:23:33Z

maybe default the setting to M20 and on capabilities report read programmatically update the setting automatically? Basically make it part of the firmware discovery process, which I think is used for other settings IIRC.

src/octoprint/util/comm.py

foosel

The capability report detection logic should really be implemented analog to the firmware information detection logic. So, add another boolean flag self._firmware_capabilities_received next to the declaration of self._firmware_info_received. A capability report is ongoing if self._firmware_capabilities has at least one entry (one cap: line was read and parsed into it). So adding some check like this in the monitor loop:

if self._firmware_capabilities and not self._firmware_capabilities_received and not lower_line.startswith("cap:"):
    self._firmware_capabilities_received = True

    # fire off an event?

    if self._defer_sd_list:
        # SD listing was delayed, do it now
        self.refreshSdFiles()

should track nicely whether we have received the full capability report. self._defer_sd_list should be set to settings().getBoolean(["serial", "waitToLoadSdFileList"]) once in the constructor, similar to most of the other config parameters.

kForth · 2022-08-02T20:20:19Z

Thanks for the input, I've refactored my code to better match the firmware_info implementation and added an event hook.

I also created a quick test plugin for the hook: https://github.com/kForth/OctoPrint-AfterReportTest

Let me know what you think.

foosel

The logic looks sound now, thank you! Only issue I have now is the name of the new hook 😅

after_report is a bit to generic. capabilities is already taken for the individual capability lines, but I think octoprint.comm.protocol.firmware.capability_report would be fairly self descriptive without any ambiguity as to what report we are talking about.

In hindsight it would admittedly have been better to parse the full report first, then call plugins, instead of introducing this individual capabilities hook in the first place, but well, we are stuck with that now.

edit Also please make sure you have pre-commit installed on your end, your changes currently don't pass its checks due to a typo as seen in the CI results.

foosel · 2022-08-18T13:39:19Z

@kForth please request a new review once you deem this PR ready for one

foosel · 2022-09-07T14:24:50Z

FYI, seen the review request, it's on my TODO list, but I'm currently fully focused on 1.8.3.

foosel · 2022-09-28T10:23:38Z

Thank you for the PR, and thank you for your patience while I was busy with 1.8.x :)

Defer M20 commands until initial capability report is done

521bc26

github-actions bot added targets maintenance The PR targets the maintenance branch approved Issue has been approved by the bot or manually for further processing labels Jul 14, 2022

Fix whitepsace

7ed73b6

Add setting for deferring sd file refresh

07c64a9

jneilliii reviewed Jul 18, 2022

View reviewed changes

src/octoprint/util/comm.py Outdated Show resolved Hide resolved

jneilliii reviewed Jul 18, 2022

View reviewed changes

src/octoprint/util/comm.py Outdated Show resolved Hide resolved

jneilliii reviewed Jul 18, 2022

View reviewed changes

src/octoprint/util/comm.py Show resolved Hide resolved

kForth marked this pull request as draft July 18, 2022 13:04

cp2004 linked an issue Jul 18, 2022 that may be closed by this pull request

The first M20 done on connection is missing the L flag. #4493

Closed

4 tasks

foosel self-requested a review August 1, 2022 09:44

foosel requested changes Aug 2, 2022

View reviewed changes

foosel assigned kForth Aug 2, 2022

kForth added 2 commits August 2, 2022 15:31

Refactor to be more similar to _firmware_info implementation

99f1bac

Add event hook for after capability report

f79e63f

foosel added this to the 1.9.0 milestone Aug 3, 2022

Merge branch 'maintenance' into fix/4493

eef084e

kForth marked this pull request as ready for review August 3, 2022 18:44

foosel assigned foosel and unassigned kForth Aug 4, 2022

foosel requested changes Aug 4, 2022

View reviewed changes

foosel assigned kForth and unassigned foosel Aug 4, 2022

Improve capability report hook name

ee051f7

kForth force-pushed the fix/4493 branch from 4bced6e to ee051f7 Compare August 10, 2022 18:46

kForth requested a review from foosel August 19, 2022 15:45

Fix headline formatting

921bb3d

foosel approved these changes Sep 28, 2022

View reviewed changes

foosel merged commit 84b83b6 into OctoPrint:maintenance Sep 28, 2022

kForth deleted the fix/4493 branch February 1, 2023 04:35

github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/4493 - Defer M20 commands until initial capability report is done #4577

Fix/4493 - Defer M20 commands until initial capability report is done #4577

kForth commented Jul 14, 2022 •

edited

Loading

kForth commented Jul 16, 2022

cp2004 commented Jul 17, 2022 •

edited

Loading

jneilliii commented Jul 17, 2022

jneilliii commented Jul 17, 2022

jneilliii commented Jul 17, 2022

kForth commented Jul 18, 2022

jneilliii commented Jul 18, 2022

jneilliii commented Jul 18, 2022

foosel left a comment

kForth commented Aug 2, 2022

foosel left a comment •

edited

Loading

foosel commented Aug 18, 2022

foosel commented Sep 7, 2022

foosel commented Sep 28, 2022

Fix/4493 - Defer M20 commands until initial capability report is done #4577

Fix/4493 - Defer M20 commands until initial capability report is done #4577

Conversation

kForth commented Jul 14, 2022 • edited Loading

What does this PR do and why is it necessary?

How was it tested? How can it be tested by the reviewer?

What are the relevant tickets if any?

Screenshots (if appropriate)

Further notes

kForth commented Jul 16, 2022

cp2004 commented Jul 17, 2022 • edited Loading

jneilliii commented Jul 17, 2022

jneilliii commented Jul 17, 2022

jneilliii commented Jul 17, 2022

kForth commented Jul 18, 2022

jneilliii commented Jul 18, 2022

jneilliii commented Jul 18, 2022

foosel left a comment

Choose a reason for hiding this comment

kForth commented Aug 2, 2022

foosel left a comment • edited Loading

Choose a reason for hiding this comment

foosel commented Aug 18, 2022

foosel commented Sep 7, 2022

foosel commented Sep 28, 2022

kForth commented Jul 14, 2022 •

edited

Loading

cp2004 commented Jul 17, 2022 •

edited

Loading

foosel left a comment •

edited

Loading