Shared AutoReport class for Temperature, Cardreader #20913

thinkyhead · 2021-01-29T01:31:16Z

Auto-report is a common paradigm in Marlin, so encapsulate this behavior as a simple template class.
Also apply macros like SEC_TO_MS where contextually-appropriate.

lightmaster · 2021-01-29T19:24:33Z

This one has broken Temp reporting for me. I rolled back to the commit before this one, and was able to get temps in Octoprint again.

)" This reverts commit 9d0e64a.

X-Ryl669 · 2021-01-30T14:58:15Z

Marlin/src/sd/cardreader.h

+    // SD Auto Reporting
+    //
+    #if HAS_MULTI_SERIAL
+      static serial_index_t auto_report_port;


This is wrong as a static variable is not a constexpr so it can't be used as a template parameter.

X-Ryl669 · 2021-01-30T15:02:44Z

Marlin/src/libs/autoreport.h

+    const millis_t ms = millis();
+    if (ELAPSED(ms, next_report_ms)) {
+      next_report_ms = ms + SEC_TO_MS(report_interval);
+      PORT_REDIRECT(AR_PORT_INDEX);


Beware, since the serial hooking code, PORT_REDIRECT expect a mask, not an index, you should use PORT_REDIRECT(SERIAL_MASK(AR_PORT_INDEX)) here instead.
There is no need for template parameter here, the mask is a runtime variable, it can be changed anytime!

X-Ryl669 · 2021-01-30T15:04:52Z

Marlin/src/sd/cardreader.h

+      static constexpr serial_index_t auto_report_port = 0;
+    #endif
+    class AutoReportSD : public AutoReporter<auto_report_port> { void auto_report(); };
+    static AutoReportSD auto_reporter;
  #endif


I think, there is no need for a template AutoReporter here, just set the serial_index_t as a member of the class since it's only used for PORT_REDIRECT which is a runtime check, and you can also remove the #if HAS_MULTI_SERIAL condition here so it'll be simpler.

X-Ryl669 · 2021-01-30T15:10:26Z

Marlin/src/module/temperature.h

-          auto_report_temp_interval = v;
-          next_temp_report_ms = millis() + 1000UL * v;
-        }
+        class AutoReportTemp : public AutoReporter<SERIAL_ALL> { void auto_report(); };


auto_report is not virtual so this is never called when manipulating the base class.

If you want to avoid the cost of a virtual table (since the instance is known at compile time), you can use CRTP here:

template <class Child> struct AutoReporter { [...] inline void auto_report() { static_cast<Child*>(this)->report(); } // Call the child's version. Does not need to be called auto_report [...] }; // You need a default implementation that does not report. struct NoAutoReport : public AutoReporter<NoAutoReport> { void report() {} }; // Then subclass like this: struct AutoReportTemp : public AutoReporter<AutoReportTemp> { void report() { ... } };

But this means you'll have to typedef the actual class to use since the type will change on each platform:

// In some common header, select the type to use based on compile time information: #ifdef HAS_MULTI_SERIAL typedef AutoReportMulti AutoReport; #else if ... typedef NoAutoReport AutoReport; [...] #endif // Ok, let's declare the reporter instance extern AutoReport auto_reporter;

X-Ryl669 · 2021-01-30T15:20:05Z

@lightmaster : Can you replace the line 33 from autoreport.h from:

 inline void auto_report() { }

to

virtual void auto_report() {}

It should fix it.

benlye · 2021-01-30T16:34:31Z

@lightmaster : Can you replace the line 33 from autoreport.h from:
 inline void auto_report() { }
to
virtual void auto_report() {}
It should fix it.

Confirmed that this change fixes temperature auto-reporting for me. (BTT SKR 1.4 Turbo, if it matters.)

Mezabaru · 2021-01-30T18:24:13Z

@lightmaster : Can you replace the line 33 from autoreport.h from:
 inline void auto_report() { }
to
virtual void auto_report() {}
It should fix it.
Confirmed that this change fixes temperature auto-reporting for me. (BTT SKR 1.4 Turbo, if it matters.)

Worked for me, ender 3 pro with skr mini E3

Guilouz · 2021-01-31T02:34:45Z

When I enable #define AUTO_REPORT_SD_STATUS, I can't compile, error :

When disable it it's ok

X-Ryl669 · 2021-01-31T07:11:52Z

@Guilouz Yes, it's known. It'll be fixed soon.

X-Ryl669 · 2021-01-31T23:28:08Z

@Guilouz Can you try #20959 (it should fix your build issue) and report ?

Guilouz · 2021-01-31T23:48:50Z

@Guilouz Can you try #20959 (it should fix your build issue) and report ?

Confirmed, it's woking !

X-Ryl669 · 2021-01-31T23:59:06Z

Thanks!

Guilouz · 2021-02-01T00:01:39Z

@X-Ryl669

But it's failed when I try to disable #define SDSUPPORT

Working with stable build 2.7.0.2 not with bugfix.

X-Ryl669 · 2021-02-01T00:04:20Z

Seems like it related #20846, that's another bug. Can you open a new issue ?

Guilouz · 2021-02-01T00:08:14Z

#20846

Yes I can.

…mware#20913)"" This reverts commit f301646.

…mware#20913)"" This reverts commit ae0eeb2.

Apply SEC_TO_MS macro

a66efa5

thinkyhead added the PR: Coding Standards label Jan 29, 2021

Shared AutoReport class for Temperature, Cardreader

3909cea

thinkyhead force-pushed the bf2_time_and_autoreport_PR branch from 58feb92 to 3909cea Compare January 29, 2021 01:41

thinkyhead merged commit 9d0e64a into MarlinFirmware:bugfix-2.0.x Jan 29, 2021

thinkyhead deleted the bf2_time_and_autoreport_PR branch January 29, 2021 02:40

lightmaster mentioned this pull request Jan 29, 2021

[BUG] Temperature no longer reported to OctoPrint #20921

Closed

mluckau added a commit to mluckau/MarlinTronxyX5SA-Pro that referenced this pull request Jan 29, 2021

Revert "AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913

f301646

)" This reverts commit 9d0e64a.

soularus added a commit to soularus/Marlin that referenced this pull request Jan 30, 2021

Revert "AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913

ae0eeb2

)" This reverts commit 9d0e64a.

X-Ryl669 reviewed Jan 30, 2021

View reviewed changes

GMagician mentioned this pull request Jan 30, 2021

Refactor serial class with templates #20783

Merged

X-Ryl669 pushed a commit to X-Ryl669/MarlinForMKS that referenced this pull request Jan 31, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

3033e8f

X-Ryl669 mentioned this pull request Jan 31, 2021

Fix AutoReporter not building and not working #20959

Merged

mluckau added a commit to mluckau/MarlinTronxyX5SA-Pro that referenced this pull request Feb 1, 2021

Revert "Revert "AutoReport class (Temperature, Cardreader) (MarlinFir…

e2e10f5

…mware#20913)"" This reverts commit f301646.

Jyers pushed a commit to Jyers/Marlin that referenced this pull request Feb 3, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

9e400d5

soularus added a commit to soularus/Marlin that referenced this pull request Feb 4, 2021

Revert "Revert "AutoReport class (Temperature, Cardreader) (MarlinFir…

c953402

…mware#20913)"" This reverts commit ae0eeb2.

kpishere pushed a commit to kpishere/Marlin that referenced this pull request Feb 19, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

54bc1cb

zillarob pushed a commit to zillarob/Marlin that referenced this pull request Feb 25, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

bb8112d

W4tel-BiDi pushed a commit to W4tel-BiDi/Marlin that referenced this pull request Apr 5, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

ed0f2f4

thinkyhead added a commit to thinkyhead/Marlin that referenced this pull request Apr 29, 2021

AutoReport class (Temperature, Cardreader) (MarlinFirmware#20913)

c7e581f

thinkyhead added a commit that referenced this pull request Apr 30, 2021

AutoReport class (Temperature, Cardreader) (#20913)

4b9f2f1

thinkyhead mentioned this pull request Dec 25, 2021

Refactor HAL as singleton #23295

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shared AutoReport class for Temperature, Cardreader #20913

Shared AutoReport class for Temperature, Cardreader #20913

thinkyhead commented Jan 29, 2021

lightmaster commented Jan 29, 2021

X-Ryl669 Jan 30, 2021

X-Ryl669 Jan 30, 2021

X-Ryl669 Jan 30, 2021

X-Ryl669 Jan 30, 2021

X-Ryl669 Jan 30, 2021

X-Ryl669 commented Jan 30, 2021 •

edited

Loading

benlye commented Jan 30, 2021 •

edited

Loading

Mezabaru commented Jan 30, 2021

Guilouz commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

Guilouz commented Jan 31, 2021 •

edited

Loading

X-Ryl669 commented Jan 31, 2021

Guilouz commented Feb 1, 2021 •

edited

Loading

X-Ryl669 commented Feb 1, 2021

Guilouz commented Feb 1, 2021

Shared AutoReport class for Temperature, Cardreader #20913

Shared AutoReport class for Temperature, Cardreader #20913

Conversation

thinkyhead commented Jan 29, 2021

lightmaster commented Jan 29, 2021

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

X-Ryl669 Jan 30, 2021

Choose a reason for hiding this comment

X-Ryl669 commented Jan 30, 2021 • edited Loading

benlye commented Jan 30, 2021 • edited Loading

Mezabaru commented Jan 30, 2021

Guilouz commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

X-Ryl669 commented Jan 31, 2021

Guilouz commented Jan 31, 2021 • edited Loading

X-Ryl669 commented Jan 31, 2021

Guilouz commented Feb 1, 2021 • edited Loading

X-Ryl669 commented Feb 1, 2021

Guilouz commented Feb 1, 2021

X-Ryl669 commented Jan 30, 2021 •

edited

Loading

benlye commented Jan 30, 2021 •

edited

Loading

Guilouz commented Jan 31, 2021 •

edited

Loading

Guilouz commented Feb 1, 2021 •

edited

Loading