octoprint: Fix serial output from chamber power to expected chamber temp format#4671
octoprint: Fix serial output from chamber power to expected chamber temp format#4671bkerler wants to merge 1 commit intoprusa3d:masterfrom
Conversation
|
I am not an expert on these values, but does software expect it to be in the form All other temperatures seem to be in that format. https://reprap.org/wiki/G-code#M105:_Get_Extruder_Temperature seems to be a little vague on this:
The ambient temperature is being reported as |
|
@ulab Good point, thanks ! I've changed the PR to reflect the expected format. |
|
The format seems to accept both C:49.3 and C:49.3/39.3 .... but as we already have a function to get the target chamber temperature, it does sound right to also add it to be parsed :) |
| auto current_chamber_temperature = buddy::chamber().current_temperature(); | ||
| if (current_chamber_temperature.has_value()) SERIAL_ECHOPAIR(" C@:", current_chamber_temperature.value()); | ||
| auto target_chamber_temperature = buddy::chamber().target_temperature(); | ||
| if (current_chamber_temperature.has_value()) SERIAL_ECHOPAIR(" C:", current_chamber_temperature.value(), "/", target_chamber_temperature.value()); |
There was a problem hiding this comment.
Do you think ECHOPAIR will work with four parameters?
There was a problem hiding this comment.
Yes, at least there are several codelines using multiple arguments (see binary_protocol.h, line 170).
But for readability, I've now split it up to two SERIAL_ECHOPAIR :)
| auto current_chamber_temperature = buddy::chamber().current_temperature(); | ||
| if (current_chamber_temperature.has_value()) SERIAL_ECHOPAIR(" C@:", current_chamber_temperature.value()); | ||
| auto target_chamber_temperature = buddy::chamber().target_temperature(); | ||
| if (current_chamber_temperature.has_value()) SERIAL_ECHOPAIR(" C:", current_chamber_temperature.value(), "/", target_chamber_temperature.value()); |
There was a problem hiding this comment.
Not checking whether target_chamber_temperature has value
| auto target_chamber_temperature = buddy::chamber().target_temperature(); | ||
| if (current_chamber_temperature.has_value()){ | ||
| SERIAL_ECHOPAIR(" C:", current_chamber_temperature.value()); | ||
| SERIAL_ECHOPAIR("/", target_chamber_temperature.has_value() ? target_chamber_temperature.value() : 0.0f); |
There was a problem hiding this comment.
There is std::optional::value_or. In that case, you don't even need to store target_chamber_temperature into a separate variable
| auto current_chamber_temperature = buddy::chamber().current_temperature(); | ||
| if (current_chamber_temperature.has_value()) SERIAL_ECHOPAIR(" C@:", current_chamber_temperature.value()); | ||
| auto target_chamber_temperature = buddy::chamber().target_temperature(); | ||
| if (current_chamber_temperature.has_value()){ |
There was a problem hiding this comment.
Nitpick/tip: You can do if(auto temp = buddy::chamber().current_temperature())
af2baa1 to
fbb6a03
Compare
|
Would love to see this merged and released! In discussion with some octo contributors it looks good. |
|
Hi, thanks for this work! |
|
Thank you @bkerler ❤️ |
FYI: Jenkins has been disabled on public PRs. |
|
@bkerler Can you please check if that hasn't been implemented in 6.4.0-alpha or 6.4.0-rc? |
|
Sure ! Will have a look |
|
The RC seems to still have the old one: |
|
@3d-gussner as @BlueFyre correctly already stated, the RC still does have the old behaviour and thus isn't fixed. |
|
Same issue for C1L |
…d target temperature
f5020fd to
b2763c4
Compare
This changes the C@: output which shall be used for chamber power only to C:x.x/m.m where x is current chamber temp and m is target chamber temp in order to match the expected behaviour, see reference. Also separated from chamber power define to allow chamber temp being used if chamber api and chamber temp is supported, even if chamber power isn't defined.
As HAS_HEATER_CHAMBER and HAS_TEMP_CHAMBER are old Merlin defines and HAS_CHAMBER_API() is used in favor, we prioritize HAS_CHAMBER_API for now until the old Merlin code has been removed.