Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/PanelDue #798

Closed
wants to merge 30 commits into from

Conversation

Projects
None yet
5 participants
@chrisbrent
Copy link

commented Dec 10, 2015

Starting a pull request to let you know I'm working on this. Interested in thoughts on handling the various types that might be requested. It's going to be one nasty case statement right now :)
Started case statement for various response types as per:
http://reprap.org/wiki/G-code#M408:_Report_JSON-style_response
and
http://reprap.org/wiki/Proposed_RepRap_Duet_Status_Responses#Status_Response_Definitions

// Beginning
gcode->stream->printf("{\"status\":\"I\",");
gcode->stream->printf("\"heaters{{\":[");
for(auto it = THEKERNEL->temperature_control_pool->get_controllers().begin();

This comment has been minimized.

Copy link
@wolfmanjm

wolfmanjm Dec 10, 2015

Contributor

This is not the correct way to keep a list of temperature controllers, there is a better way and it is used in several other modules. so you will need to change this to comply with the way it is currently done. It also wastes a lot of memory.

@@ -29,6 +29,7 @@ void TemperatureControlPool::load_tools()
// If module is enabled
if( THEKERNEL->config->value(temperature_control_checksum, cs, enable_checksum )->as_bool() ) {
TemperatureControl *controller = new TemperatureControl(cs, cnt++);
controllers.push_back( cs );

This comment has been minimized.

Copy link
@wolfmanjm

wolfmanjm Dec 10, 2015

Contributor

remove this and all other references to it.

@@ -56,6 +57,7 @@ class Kernel {
Planner* planner;
Config* config;
Conveyor* conveyor;
TemperatureControlPool* temperature_control_pool;

This comment has been minimized.

Copy link
@wolfmanjm

wolfmanjm Dec 10, 2015

Contributor

remove this

@wolfmanjm

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2015

Both the panel and the temperature switch handle getting data from all temperature controls, so please use that method, and not add additional, unnecessary overhead to do the same thing. Thank you.
If you find you need to make changes to anything other than your module then you are missing something, and you can ask us how to do it.

@wolfmanjm

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2015

Plus temperaturecontrolpool does not exist once the config part of the boot has been finished, so this would never have worked anyway you would simply crash.

@@ -13,6 +13,10 @@
class TemperatureControlPool {
public:
void load_tools();
const std::vector<uint16_t>& get_controllers() const { return controllers; };

This comment has been minimized.

Copy link
@wolfmanjm

wolfmanjm Dec 10, 2015

Contributor

remove this

@wolfmanjm

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2015

This is how you access all temperature controllers... (from SimpleShell)

            // scan all temperature controls
            std::vector<struct pad_temperature> controllers;
            bool ok = PublicData::get_value(temperature_control_checksum, poll_controls_checksum, &controllers);
            if (ok) {
                for (auto &c : controllers) {
                   stream->printf("%s (%d) temp: %f/%f @%d\r\n", c.designator.c_str(), c.id, c.current_temperature, c.target_temperature, c.pwm);
                }

            } else {
                stream->printf("no heaters found\r\n");
            }

Chris Brent added some commits Dec 10, 2015

@chrisbrent

This comment has been minimized.

Copy link
Author

commented Dec 10, 2015

Thanks, sorry for the poor code. I originally started doing this off Smothie2 as Arthur posted a link to that in a G+ post. This looks better I think? M408 the following:

heaters: current heater temperatures, numbered as per the machine (typically, heater 0 is the bed)
active: active temperatures of the heaters
standby: standby temperatures of the heaters
hstat: status of the heaters, 0=off, 1=standby, 2=active, 3=fault

Do you know if "active" is the target temp?
It looks like Smoothie doesn't have the concept of a standby temperature?
The TemperatureControl doesn't have a status, is there a class to us for this?

@wolfmanjm

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2015

There is no standby temperature for smoothie so you would ignore that.
You can determine the status of the heater from the returned data.
So if target_temperature is 0 it is off, if target_temperature is > current_temperature then it is heating if target_temperature is equal or close to the current_temperature then it has reached its temperature.

Chris Brent added some commits Dec 10, 2015

Chris Brent
Chris Brent
Add config to load Reporter module. Add status, see code for assumpti…
…on of M408 S0 status mapping to Smoothie data.
Chris Brent

@wolfmanjm wolfmanjm added the WIP label Dec 14, 2015

@chrisbrent chrisbrent referenced this pull request Dec 14, 2015

Closed

Update Reporter.cpp #6

std::vector<struct pad_temperature> controllers;
if (PublicData::get_value(temperature_control_checksum, poll_controls_checksum, &controllers)) {
for (auto &c : controllers) {
if(&c == &controllers.back()){

This comment has been minimized.

Copy link
@wolfmanjm

wolfmanjm Dec 14, 2015

Contributor

not sure I understand what this is, the results are identical.

This comment has been minimized.

Copy link
@chrisbrent

chrisbrent Dec 15, 2015

Author

It's supposed to do the , correctly for the Json array but I copy and pasted poorly. I need to rethink this anyway as the heater array for M408 says "current heater temperatures, numbered as per the machine (typically, heater 0 is the bed)".

if(c.target_temperature>c.current_temperature){
gcode->stream->printf("2");
}
if(&c != &controllers.back()){

This comment has been minimized.

Copy link
@wolfmanjm

wolfmanjm Dec 14, 2015

Contributor

This is really not a good way to determine if it is the last one. maybe use an index instead

gcode->stream->printf("0");
}
// active
if(c.target_temperature>c.current_temperature){

This comment has been minimized.

Copy link
@wolfmanjm

wolfmanjm Dec 14, 2015

Contributor

This is not the correct way to determine it is active... if target_temperature is > 0 then it is active it has nothing to do with the current_temperature at all.

This comment has been minimized.

Copy link
@chrisbrent

chrisbrent Dec 14, 2015

Author

Thanks, I mixed up active and heating (which doesn't exist as a status)

gcode->stream->printf("\"probe\":\"535\",\"fanRPM\":0,\"homed\":[0,0,0],\"fraction_printed\":");
// Get fraction printed
void *completed_data;
if (PublicData::get_value( player_checksum, get_progress_checksum, &completed_data )) {

This comment has been minimized.

Copy link
@wolfmanjm

wolfmanjm Dec 14, 2015

Contributor

This only works if printing from an sd card.

Chris Brent and others added some commits Dec 15, 2015

Merge pull request #4 from chrisbrent/feature/due_panel
Merge edge into Feature/due panel
Merge pull request #5 from chrisbrent/edge
Latest edge to due_panel

chrisbrent and others added some commits Jan 24, 2016

Merge pull request #7 from Smoothieware/edge
Merge edge into Feature/due panel
Chris Brent
Merge pull request #10 from Smoothieware/edge
Getting Edge back in sync with my branch
@chrisbrent

This comment has been minimized.

Copy link
Author

commented Mar 15, 2016

@626Pilot This is a lot more complete now. It's merged with the current edge so if you can build this give it a try.

@chrisbrent chrisbrent changed the title Feature/due panel Feature/PanelDue Mar 15, 2016

@chrisbrent

This comment has been minimized.

Copy link
Owner

commented on src/modules/tools/reporter/Reporter.cpp in 15859b9 Mar 15, 2016

@wolfmanjm I don't like this, and I know it fails for multiple hotends. Is there a better way to get the designator? I see that the TemperatureControlPool constructor does it, but it uses a uint of "name" and I don't see an obvious way to get all that into a module?

This comment has been minimized.

Copy link

replied Mar 15, 2016

look at the panel watch screen it handles multiple temperature controls.

@626Pilot

This comment has been minimized.

Copy link

commented Mar 16, 2016

@chrisbrent I just merged with upstream, but your code isn't there. I guess that's because it's in a feature. I'll wait until the code is checked into the main repo, and there is some documentation. That said, thanks for keeping me in the loop! :)

@arthurwolf

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

@chrisbrent Where is this at ? Anything blocking you ?

@chrisbrent

This comment has been minimized.

Copy link
Author

commented Jun 28, 2016

It was going fine, but then I noticed that every now and then the Panel
stops responding, when I reset it, it just stays connecting even though
M408 responds fine in Pronterface, etc. I need to work out how to
troubleshoot that. I'd also like to finalize getting it onto its own serial
instead of using the debug console. That just stopped because I couldn't
work out how to get serial on arbitrary pins using the configfile, or
which pins I should take. It would also be good to have other people using
this as I only have one hotend, don't print from it's SD card much etc.
Any tips on these would be great, thanks.

On Tue, Jun 28, 2016 at 1:49 PM, Arthur Wolf notifications@github.com
wrote:

@chrisbrent https://github.com/chrisbrent Where is this at ? Anything
blocking you ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#798 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAihOgIOn4Envii1YEnL5Mu3ItuzhCvLks5qQYjOgaJpZM4GycgR
.

@arthurwolf

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2016

@chrisbrent The pins that are normally used for the RRD GLCD panel ( the SPI connector right next to the microcontroller ) can also be used as a UART ( UART1 ) just by creating a new SerialConsole object using those pins.
Like this : https://github.com/Smoothieware/Smoothieware/blob/edge/src/libs/Kernel.cpp#L60
Don't forget to register it as a module : https://github.com/Smoothieware/Smoothieware/blob/edge/src/libs/Kernel.cpp#L106
And then it should be able to work just like UART0 does now.

@chrisbrent

This comment has been minimized.

Copy link
Author

commented Jun 28, 2016

Yes, I just worked that out :) Sorry it should be obvious by now!

On Tue, Jun 28, 2016 at 2:15 PM, Arthur Wolf notifications@github.com
wrote:

@chrisbrent https://github.com/chrisbrent The pins that are normally
used for the RRD GLCD panel ( the SPI connector right next to the
microcontroller ) can also be used as a UART ( UART1 ) just by creating a
new SerialConsole object using those pins.
Like this :
https://github.com/Smoothieware/Smoothieware/blob/edge/src/libs/Kernel.cpp#L60
Don't forget to register it as a module :
https://github.com/Smoothieware/Smoothieware/blob/edge/src/libs/Kernel.cpp#L106
And then it should be able to work just like UART0 does now.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#798 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAihOsnH1xWFk6kuguCvOtx4K5L6UEFFks5qQY7igaJpZM4GycgR
.

chrisbrent added some commits Jun 29, 2016

Merge branch 'edge'
Conflicts:
	src/main.cpp
@@ -172,7 +172,17 @@ void init() {
kernel->add_module( new Spindle() );
#endif
#ifndef NO_UTILS_PANEL
<<<<<<< HEAD

This comment has been minimized.

Copy link
@wolfmanjm

wolfmanjm Jul 6, 2016

Contributor

can't merge something that does not compile... you need to fix these messages.

@wolfmanjm

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2016

I'm going to close this until it is fixed. RE open when it builds cleanly.

@AHHams

This comment has been minimized.

Copy link

commented Oct 20, 2016

Is anyone still working on this? Chris?

@chrisbrent

This comment has been minimized.

Copy link
Author

commented Oct 25, 2016

Yes, I just need to find two hours to fix one bug and get the merge sorted.
But finding that time is tricky.

On Oct 20, 2016 12:52, "AHHams" notifications@github.com wrote:

Is anyone still working on this? Chris?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#798 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAihOjy8wc7BxMqNqw73hs6ZZ9upF4SRks5q18Z4gaJpZM4GycgR
.

@AHHams

This comment has been minimized.

Copy link

commented Oct 28, 2016

I fixed the issue with main.ccp. It is in a feature/due_panel branch on my fork. It works and is otherwise in sync with latest edge.

My PanelDue seems to work fine up till now, but I did only very testing to date. Reading / setting temps works, jogging works. Did not do any prints yet.

I did have to update the firmware on the PanelDue to latest 1.15c. I did not get it to connect with prior version.

I am new to git/Github. Don't know proper etiquette and process for contributing to pull requests. Let me know if there's anything further I can do to help get this fix into latest edge.

}else{
gcode->stream->printf("P");
}
if (THEKERNEL->is_halted()) {

This comment has been minimized.

Copy link
@AHHams

AHHams Oct 28, 2016

This would result in "IS" if printer is idle and halted?

gcode->stream->printf("S");
}
void *returned_data;
if (PublicData::get_value( player_checksum, is_suspended_checksum, &returned_data )) {

This comment has been minimized.

Copy link
@AHHams

AHHams Oct 28, 2016

This would result in "PA" if queue not empty and paused? Suggest to make code a little safer, so only "I", "P", "S", or "A" are returned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.