Skip to content

Commit

Permalink
Enable ISRs inside temperature ISR
Browse files Browse the repository at this point in the history 
to capture chars at UART at 250000 baud.
  • Loading branch information
Sebastianv650 committed Nov 28, 2016
1 parent 5005969 commit 912704a
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions Marlin/temperature.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1489,6 +1489,9 @@ void Temperature::set_current_temp_raw() {
ISR(TIMER0_COMPB_vect) { Temperature::isr(); }

void Temperature::isr() {
//Allow UART and stepper ISRs
CBI(TIMSK0, OCIE0B); //Disable Temperature ISR
sei();

static uint8_t temp_count = 0;
static TempState temp_state = StartupDelay;
Expand Down Expand Up @@ -1940,4 +1943,6 @@ void Temperature::isr() {
if (!endstop_monitor_count) endstop_monitor(); // report changes in endstop status
}
#endif

SBI(TIMSK0, OCIE0B); //re-enable Temperature ISR
}

10 comments on commit 912704a

@daid
Copy link
Contributor

@daid daid commented on 912704a Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next to this being super super super dangerous, you most likely broke stepping when baby stepping is used.

I would recommend actually fixing the core problem if doing to much unneeded shit in the interrupt routine.

@Sebastianv650
Copy link
Contributor

@Sebastianv650 Sebastianv650 commented on 912704a Mar 2, 2017 •
edited by thinkyhead
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is an issue with babystepping, please provide the issue number.

If you find unneded shit in the ISR, feel free to create an issue with a proposal or create a PR - nobody will keep this ISR thing if there is a better solution.
In fact, there was no solution when I did this changes and it solves all problems I had like communication errors, followed by hiccups due to buffer underrun, wired printer movements and so on.

@daid
Copy link
Contributor

@daid daid commented on 912704a Mar 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you most likely fixed buffering problems, and introduced a whole bunch of race conditions in the process that will influence stability. I'm just saying you are trading one problem for another.

I'm just warning you. I'm not using this version of Marlin. We never had buffering problems in the older fork of Marlin I'm using at 250000. You're free to ignore my warning. I just flew by because I wanted to check something else and noticed this commit by chance.

But I can give you one example of where you are pretty much messing thing up, and that's the whole endstop monitoring function which sends data out on serial.
The baby stepping is going to give problems because that function pretty much looks like designed to be ran without being interrupted from the stepper interrupt. As they are both accessing the same pins.

@Sebastianv650
Copy link
Contributor

@Sebastianv650 Sebastianv650 commented on 912704a Mar 2, 2017 •
edited by thinkyhead
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can read the whole problem in #3680. The basic problem is quite simple to understand, but there are only two options to solve it:

  • Shorten all ISRs. The opposite is the reality due to mixing extruders, pressure compensation and other features you older fork might not have.
  • Use lower baud rate. This is only possible up to a specific degree as Marlin needs data to process.
  • Allow the serial UART ISR to interrupt other ISRs. That's what's the case at the moment.

Up to now I have no issue in mind where the endstop monitoring or babystepping is broken. But you are right that this block of code inside the temperature ISR should be protected by further stepper ISRs.
Can you be more specific to the endstop monitoring function? I don't see how this should be affected.

@daid
Copy link
Contributor

@daid daid commented on 912704a Mar 2, 2017 •
edited by thinkyhead
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying the problem isn't real. I'm just stating that your solution is wrong and will cause you more pain then gain.

The problem with baby stepping is that the baby stepping can now be interrupted by the stepper, where they both work on the same IOs. Giving two "masters".

The problem with endstop monitoring is that it does serial output, which takes a long time, causing the interrupt to block and the whole reason you are having problems is that interrupts are taking too long. So that is one source of your problems.

If nozzle-mixing and pressure-compensation are done in interrupts then the problems are bigger than I thought and I'd better shut up. (I am wondering what the maximum step rate is now. As this is one of the problems we ARE running into with our current Marlin code)

@Sebastianv650
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying the problem isn't real. I'm just stating that your solution is wrong and will cause you more pain then gain.

Nope. It's running just fine on my printer. As stated before, if you have a better idea or a PR, we will be open for everything. If not, don't complain about fixes that are not nice but the best the Marlin community has up to now.

Regarding babystepping I will create a Issue to discuss two options I have in mind.

The problem with endstop monitoring is that it does serial output, which takes a long time, causing the interrupt to block and the whole reason you are having problems is that interrupts are taking to long. So that is one source of your problems.

PINS_DEBUGGING, as the name is saying it, is for debugging purposes and not for production print runs. Therefore my oppinion is that it's OK to do serial outputs within an ISR. Beside that, this feature is not related to this PR. It was also sending over serial before.

If nozzle mixing and pressure compensation are done in interrupts then the problems are bigger then I thought and I better shut up.

Everything that can be done is calculated inside the planner and added to the block buffer. But there are things that can't be calculated in advance, this are calculated inside the stepper ISR. Again, if you have ideas for improvements or PRs, just open a ticket..

I am wondering what the maximum step rate is now. As this is one of the problems we ARE running into with our current Marlin code

There is no one fixed step rate. Marlin has a lot of features that adds additions load to the processor, therefore the real maximum step rate will vary.
For example on a plain non-kinematic printer without fancy features, LCD and other things enabled it should run as fast or maybe even faster than Marlin 1.0.
If you use a CORE-kinematic, together with MBL, maybe a mixing extruder, graphical LCD and so on you will most likely not reach the 40kHz step rate. For this reason, there is an active discussion about a future 32bit board for Marlin.

@daid
Copy link
Contributor

@daid daid commented on 912704a Mar 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UM2 with graphical LCD and SD support has no problems with the 40k step rate. CoreXY should not effect this. Delta has a problem but that's more related to planner speed.
I have experiments with 60khz step rates, the hard limit seems to be around ~65khz on 5 steppers, then you simply cannot work with 5 steppers and do the required instructions with interrupt overhead and endstop sampling, and still handle the UART. Even if you strip everything.

Like I said, I don't care what you do with this code, just warning you. You are free to ignore my warning.

"It works for me" isn't really the best way forward in highly time critical embedded code with this many configuration options IMHO.

In the end, you're not hurting me, or any of the products I support. You might only hurt yourself or the community using this version of Marlin.

@thinkyhead
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

introduced a whole bunch of race conditions in the process

True. Race conditions are exactly what we're up against now.

@CONSULitAS
Copy link
Contributor

@CONSULitAS CONSULitAS commented on 912704a Mar 3, 2017 •
edited by thinkyhead
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 from me too!

@Sebastianv650 opened a discussion on #5946

I would vote for safety first. Never give race conditions a chance when working with hot hardware...

@thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented on 912704a Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I am wondering what the maximum step rate is now….)

I would also be curious about this. And, is it better or worse than Marlin 1.0.x?

Please sign in to comment.