Bug or design flaw in RC1/RC2 with I2C HW based items

[justmyopinion]

If you have a I2C display with buttons any update of LCD and button reading is made via TWI (TwoWire) and Wire library. In Marlin these button device services are slowed down by switch LCD_HAS_SLOW_BUTTONS so reading is not done in Interrupt routines but during LCD update. However LCD updates and encoder readings are normally scheduled to be serviced with display updates (100ms in RC1) in lcd_update(), but this has obviously missed with slow buttons as they are serviced in lcd_update each time lcd_update is called, and that means with a frequency of 15kHz (70uS). this is of course not suitable as TWi uses Wire library which is interrupt-based so 10000 interrupts will be issued for polling "slow buttons", which of course take an extra load from processor as each ISR is approx. 10µs. I suggest call to lcd_implementation_read_slow_buttons() in lcd_update() is moved to a position so it will be scheduled with the same frequency as LCD_UPDATE. I think this is an important "bug" which should be corrected in RC2.

[Wackerbarth]

I believe that the change that you suggest was addressed in commit 03f0edb "Fix high LCD status screen update frequency (PR#2655) which was incorporated in 1.1.0-RC2 on 28 Sept 2015.

RC2 was posted the following day.

[justmyopinion]

I hve not tested RC2 as it was my opinion that only update frequency by LCD_UPDATE_INTERVAL was changed, which was 100 and possible changed to something larger? The above has influence on processor load but has no direct connection to update frequency but would of couse have helped a bit when UPDATE_INTERVAL was 100.

[Wackerbarth]

That is possible. Please look at the related change and, if it doesn't resolve your issue, offer a correction that will do so.

[AnHardt]

At the moment (RC2) lcd_implementation_read_slow_buttons() is called whenever lcd_update() is called - in idle() plus at some places where we are waiting for input. That means when ever we have time for it. Normal buttons are checked with the same frequency.

Moving the call of lcd_implementation_read_slow_buttons() inside the

if (ms > next_lcd_update_ms) {

block means update with LCD_UPDATE_INTERVAL. (~ 10/s)

Moving inside a

if (lcdDrawUpdate)

block means update with 10*LCD_UPDATE_INTERVAL. (~ 1/s)

I suppose reading the twi-buttons only 10/s will feel a bit slow.

We could make a additional LCD_UPDATE_INTERVAL / 10 condition reading the buttons about 100 times a second.

Sorry I have no hardware to test with. Please try yourself and report back.

I'm quite sure this will not lower the processors load as this is mainly done while idle. When we reduce the work in idle, the loop will be called more frequently - The processor load is 100% anyway.

[justmyopinion]

It may be a bit more complicated than that.Let me say that I have already made these changes on my printer and Marlin is running flawless and very very smooth now at incredible speeds without any stuttering. My investigation on this started with this report: #157 and I have made numerous measurements with timers and scope to verify this. With TWi interface to display, RC1 had an update rate of 10 (100ms but the problem is that a status update takes close to 100ms and what dó you think happens when printer is running: LCD update time is raised up to 150ms.due to the interrupts running mostly steppers causing serios stuttering, but you also have itr's from serial, millis and wire lib etc. and that is why I am concerned on slowbuttons which is using Wire lib and does not claim fast update rates. Even in Timer1 (millis) slowbuttons are requested i do not know the reason for this but it is hardly necessary to scan buttons 1000 times/sec. i removed these and let lcd_update do the rest with a suitable schedule time, it is not buttons but encode wheel that needs fair update rate. So the main goal is to reduce ISR load from the processor. Personally i did this by these steps which are all standalone. 1: I made an optimized version of liquidTWI2 which reduced display update from 95 ms to 45ms. 2: I raised TWi/Wire clock from 100 kHz to 400 khz (even 800 kHz is no problem on my 2560.) 3: I quitted Wire lib and installed a modified TWI2 lib again and I2C lib replacing Wire with no itr.

So at the moment my display update is 14ms and 25-35 worstcase when printer is running and with LCD_UPDATE rate of 10 (100ms) or larger this a fair margin and I have no stuttering at all Mission completed.

There is still a lot of weird things in Marlin which could be optimized. i have only mentioned a few which are obvious

[AnHardt]

Please really have a look on https://github.com/MarlinFirmware/Marlin/commit/03f0edb57e921dd22304919bf9762dada09799a8

That means display updates about 1Hz - as intended.

We will be glad to see your PRs.

[justmyopinion]

I do not understand that reference, I had the understanding that only LCD_UPDATE_INTERVAL was changed in RC2 from 100 to something larger. Have I missed something i did not check?

The problem with the above optimizations are that no changes in marlin is needed, it is all made in external libs, but of course Marlin has to chane also. Next goal is to make a scheduler replacing LCD_update for all resources also LCD, there is still a lot of power in processor if properly used. :-D

[AnHardt]

Yes you missed something. The fix was not to incease LCD_UPDATE_INTERVAL but to make the char_LCD_update dependend on lcdDrawUpdate. lcdDrawUpdate is true when ether 10 * LCD_UPDATE_INTERVAL (~1/s) or a "event is detected" (buttons/encoder). That gives low load during the status screen but fast response when turning the encoder.

It was an error by me, not to change that when this was done for the graphical LCDs. This slipped thru because a 10Hz update for the (my) 4-wire-LCDs is absolutely no problem.

[justmyopinion]

Ok thanks, but I will still stick to the changes made. It does not worry you that when idle , turnaround for lcd_update is app 60uS which means that slowbuttons make an interrupt of 10us every 60uS due to Wire, which may influence available processor time even stepper power. i do not know the itr priority if any in AVR processors, but it may be considered anyway.

[AnHardt]

I tried to talk about this before - decreasing the load inside the idle loop means calling it more often. As the buttons twi-check is unconditionaly done every time this will increase the interrupt problem with this. For the possible solutions look above.

[justmyopinion]

We do not understand each other very well here. What I am saying is that if we reduce the update rate of slowbuttons we reduce the cycle stealing from slowbuttons itr's which reduce the load on other resource stealing itr's depending on priority of course.

[AnHardt]

Ok. Ok. I could try a fix. But this will be a shot into the dark. I don't have the hardware - so i need a tester - YOU. Just suggest a update rate. 100Hz?

[AnHardt]

We already have next_button_update_ms (500ms) around. Have to analyse it's use first.

[justmyopinion]

100 hz is fine with me and it is already tested as i moved call in lcd_update() (10Hz) to lcd_implementation_read_slow_buttons() into the protection of LCD update freq : if (ms > next_lcd_update_ms) { and added an extra call to lcd_buttons_update in and outside this time frame so lcd_buttons_update are still called frequently so no problems with wheel "buttons". I have used this for some days now and found no worries however I do not know the influence of changes in RC2..

[AnHardt]

Please test and report about #2690

[justmyopinion]

Which download do I need to test it?

[AnHardt]

https://github.com/AnHardt/Marlin/archive/fix2681_slowbutton_update.zip

[justmyopinion]

NO..unfortunately it did not work and it took me some time to figure out why as the, display got weird when second millis was added.to routine, if removed it worked ok. Problem is that turnaround for LCD_update is so small (60uS) so when you call millis() it will be the same for the first 16 times (1ms) and maybe Wire lib has not acknowled (itr) the first request yet (guessing). and get bulldozed. Maybe we could consider my first solutions that worked and still works? My call to 'lcd_implementation_read_slow_buttons() was put inside the millis request for lcd update so the frequency is reduced to 100ms but that is no problem really...A standard UI would normally have a debouncing time of 200ms so if you are not repeating button this is quite normal. Millis should be used with care!

It is easy to simulate by delaying button routine more than 1ms and it is not a problem in display update as display update takes oceans of time before next request,

Problems like this is why I think Marlin should have a "Real scheduler" based on millis of course but free of casual update rates.

[AnHardt]

Is #2694 what you mean?

[Roxy-3D]

Problems like this is why I think Marlin should have a "Real scheduler" based on millis of course but free of casual update rates.

I would love to see somebody slip a Real Time OS under Marlin. So many things get simpler if we could control the scheduling of activities. My guess is the motion control would be fairly high priority. The thermal stuff would probably be a more medium level priority but not use much processor power. The LCD updates could be very lowest priority but allowed to eat up any CPU cycles available.

[Wackerbarth]

Putting a Real Time OS on an AVR is problematic. Basically, we have such limited resources that we cannot implement generalized threading, etc. in the fashion that you would use on a "bigger" processor.

Fundamentally, Marlin is pushing the limit on what it is attempting to do. At some point, rather than attempting to "squeeze the last drop" out of the AVR processor, it makes better sense to just limit the functionality and start with a better processor to handle the more complex tasks.

You can already get a processor such as the BeagleBone at a cost that is not much higher that that of the 2560. And performance wise, there really isn't a comparison.

[Roxy-3D]

So... I don't really know. But my initial thoughts are that a RTOS probably helps the problem just because low priority items don't have to be checked and managed in the same loops that higher priority items have to be done. Right now everything is mixed together so that wastes some amount of processor resources. It is that line of thought that leads me to thinking it would help. But the other thing it does is it lets us separate out functions and make tasks out of them. And we get to control how often or what event wakes up a task. That gives us more control on how we budget the resources of the processor.

A while ago there were some people talking about an ARM version of Marlin. The BeagleBone processor (described here: https://en.wikipedia.org/wiki/BeagleBoard#BeagleBone) would be a welcome relief! I wonder how hard it would be to port the code over? Are there even ARM boards that have stepper motor controllers on them?

Fundamentally, Marlin is pushing the limit on what it is attempting to do. At some point, rather than attempting to "squeeze the last drop" out of the AVR processor, it makes better sense to just limit the functionality and start with a better processor to handle the more complex tasks.

Yes... But... If we do nothing, I bet 32 MHz versions of the AVR's will start shipping soon. Supposedly, there already are 50 MHz versions of the AVR but nobody is using them. Just doubling the processor speed would give us a lot of head room.

Another option would be to stick with AVR's but move to the 32 bit version. http://www.atmel.com/products/microcontrollers/avr/32-BitAVRuc3.aspx The 32 bit AVR's have a built in FPU. I have no way of knowing this for sure, but right now we are using a huge number of clock cycles doing floating point math. Doing that in hardware would help a bunch.

[Grogyan]

There is a move towards SBCs serving as print server, slicer and machine control. Which will inevitably mean, no more Arduino. Right now, stick with the limitations of the 16bit AVR Mega, and have a separate fork for 32bit AVRs like the DUE, for those who want more advanced features.

Off topic a bit, I am now superseding these tiny interface boards and using a touch display with OctoPrint. This offloads a lot of unnecessary (IMO) processing from the Arduino to the more capable Raspberry PI 2, leaving the Arduino to be solely responsible for gcode interpretation and machine control. On 10/10/2015 10:35 am, "Roxy-3DPrintBoard" notifications@github.com wrote:

So... I don't really know. But my initial thoughts are that a RTOS probably helps the problem just because low priority items don't have to be checked and managed in the same loops that higher priority items have to be done. Right now everything is mixed together so that wastes some amount of processor resources. It is that line of thought that leads me to thinking it would help. But the other thing it does is it lets us separate out functions and make tasks out of them. And we get to control how often or what event wakes up a task. That gives us more control on how we budget the resources of the processor.

A while ago there were some people talking about an ARM version of Marlin. The BeagleBone processor (described here: https://en.wikipedia.org/wiki/BeagleBoard#BeagleBone) would be a welcome relief! I wonder how hard it would be to port the code over? Are there even ARM boards that have stepper motor controllers on them?

Fundamentally, Marlin is pushing the limit on what it is attempting to do. At some point, rather than attempting to "squeeze the last drop" out of the AVR processor, it makes better sense to just limit the functionality and start with a better processor to handle the more complex tasks.

Yes... But... If we do nothing, I bet 32 MHz versions of the AVR's will start shipping soon. Supposedly, there already are 50 MHz versions of the AVR but nobody is using them. Just doubling the processor speed would give us a lot of head room.

Another option would be to stick with AVR's but move to the 32 bit version. http://www.atmel.com/products/microcontrollers/avr/32-BitAVRuc3.aspx The 32 bit AVR's have a built in FPU. I have no way of knowing this for sure, but right now we are using a huge number of clock cycles doing floating point math. Doing that in hardware would help a bunch.

— Reply to this email directly or view it on GitHub https://github.com/MarlinFirmware/Marlin/issues/2681#issuecomment-146991006 .

[Wackerbarth]

The points that you mention are true. But, right now, in addition to speed, the limiting factor is RAM. We could possibly live with a multi-threaded interrupt system if we had the resources for the parallel stacks needed to run a pre-emptive OS.

But, there isn’t really anything “special” about the AVR architecture. It should not be very difficult to port the existing algorithms to a larger processor. So, I see little reason to worry about some fancier AVR technology. Utilizing that extra capability is going to take some rework anyway. Why not just do it NOW in a different architecture. I don’t see any advantage in holding out for some future enhancement trying to turn a controller into a real computing machine.

[AnHardt]

This "MarlinFirmware/Marlin" is:

8 bit AVR without hardware floating point Arduino C, C++ and a bit of assembler

a bit anarchic a bit chaotic with two mostly vanished owners and one not using github with changing, a bit despotic main contributors with a great number of contributes over the time, every one an expert for it's own contributed part of Marlin with no one with the full overview of the code with some dozen of improvements one could implement when reading the feature requests with some dozen of errors to fix with a lot of potential for improved performance in close to every subsystem, but not the steprate it is what it is

I like it that way

If you don't like it that way - go elsewhere. If you want to change one of the first 4 point - go elsewhere.

[justmyopinion]

@AnHardt re "Is #2694 what you mean?" Exactly ,that is what is running on my printer right now and makes no trouble, thankyou for your effort, appreciate it. I assume you let lcd_buttons_update(); stay where it was. Actually this ought to be scheduled lower as well but LCD_UPDATE freq is too low for wheels so for now keep it as is. @Roxy-3DPrintBoard / @Wackerbarth : I have no ambitions of making Marlin into some RTOS so let us keep the feet on ground and get the best out of what we have. As I have said before Marlin has some great features and actually very fine stability and performance at the moment. Of course some new features still needs tuning, basically I have no interest in autolevelling as I have 3 screws on my heating bed and once levelled they are rock stable so why bother? you would be surprised if you had seen some of my measurements on processor loads. Most of the time the processor is idling but of course if you have a delta printer and try to print at 200mm/s etc you may meet the wall somewhere, but for most printers it is still possible to find resources in that little chip. When I met problems recently with excessive display updates causing printer to stutter frequently it was possible for me to find solutions that reduced display updates with a factor of 7 from around 100ms to 14 ms, so it is still posiible to do things. I will keep on my work of taking low priority jobs out of high priority scheduling (ISR's) and for this i have to keep investigating in how Marlin was born. Some key figures: stepper handling is the drive motor with highest priority and noone would possible dream to change the current sw for this, risky as it is. interrupts are very fast and short. millis timing is the base for making synchron updates with itr rates of 1 ms (1KHz Serial input itr are mostly running at 250000bit/s (25kHz) and is async in nature. to this some async itr's are added like Wire lib and similar SPI? etc. In this mess it is vital to understand the nature of how Marlin is trying to survive in this chaotic world but some of us are still looking for challenge in finding some order in the chaos and take back the control. keep up the good work folks... .

[justmyopinion]

Porting Marlin to more power has been discussed for years, here is one of them, LOL: https://github.com/MarlinFirmware/Marlin/issues/626

@Grogyan That is one way to go adding external intelligence offloading marlin and true multiprocessing :-D. This could be made on several functions, display, heating control and even stepper control etc ....,

[Roxy-3D]

AnHardt commented: with two mostly vanished owners and one not using github

So... I'm going to try to do the Sisyphus thing again and try to get GitHub to work on my machine. Don't think it is for lack of trying. I did get it to work once and M48 got merged. I'm going to start with a clean machine and see if I can do G27 when it is fully baked. (or what ever we collectively decide is the right command and number).

But here is the good news: If I get the rock almost to the top of the hill and it starts rolling down hill... I can just go edit the files in place! :) [I wouldn't really do that...]

[justmyopinion]

@AnHardt RE slowbuttons: Could you explain to me what this code is resolving, it has no meaning to me: In static uint8_t lcd_implementation_read_slow_buttons() { ...

if ENABLED(LCD_I2C_VIKI)

if ((slow_buttons & (B_MI | B_RI)) && (millis() < next_button_update_ms)) // LCD clicked slow_buttons &= ~(B_MI | B_RI); // Disable LCD clicked buttons if screen is updated

endif

[Wackerbarth]

@justmyopinion -- "I have no ambitions of making Marlin into some RTOS ..." Nor do I. However, we need to be reasonable. Relative to other parts, the cost of the electronics is the component that will continue to decrease. As more capable electronics become cheaper, it will only make sense to migrate the printer control to a more capable platform. Just as Marlin grew from GRBL and Sprinter, I am sure that the good aspects of Marlin will be ported into another generation. But that will not, and should not, be "Marlin". However, I suggest that we recognize that "it's coming" and, rather than trying to be everything to everyone, cede some of that functionality to that next generation rather than banging our heads against obstacles which limit our ability to do a excellent job rather than one that is just passable..

[AnHardt]

Indeed this looks strange. Let's begin a bit before your part.

#if ENABLED(LCD_HAS_SLOW_BUTTONS)

  extern millis_t next_button_update_ms;

  static uint8_t lcd_implementation_read_slow_buttons() {
    #if ENABLED(LCD_I2C_TYPE_MCP23017)
      uint8_t slow_buttons;
      // Reading these buttons this is likely to be too slow to call inside interrupt context
      // so they are called during normal lcd_update
      slow_buttons = lcd.readButtons() << B_I2C_BTN_OFFSET;
      #if ENABLED(LCD_I2C_VIKI)
        if ((slow_buttons & (B_MI | B_RI)) && millis() < next_button_update_ms) // LCD clicked
          slow_buttons &= ~(B_MI | B_RI); // Disable LCD clicked buttons if screen is updated
      #endif
      return slow_buttons;
    #endif
  }

#endif // LCD_HAS_SLOW_BUTTONS

Line 1) `#if ENABLED(LCD_HAS_SLOW_BUTTONS)`` LCD_HAS_SLOW_BUTTONS is defined for ether LCD_I2C_VIKI or LCD_I2C_PANELOLU2. These are defined directly in the Configuration.h's.

Line 3) extern millis_t next_button_update_ms; Allow access to next_button_update_ms. This is increased by 500, every time lcd_quick_feedback() is called. It is evaluated in lcd_buttons_update() in the context of NEWPANEL && BTN_ENC if (millis() > next_button_update_ms && READ(BTN_ENC) == 0) newbutton |= EN_C; store something when time is > next_button_update_ms newbutton is later combined with slow_buttons to buttons.

Line 5) static uint8_t lcd_implementation_read_slow_buttons() { no parameters. return a byte.

Line 6) #if ENABLED(LCD_I2C_TYPE_MCP23017) LCD_I2C_TYPE_MCP23017 is defined in Conditionals.h for LCD_I2C_VIKI or LCD_I2C_PANELOLU2 . Seems to be redundant to LCD_HAS_SLOW_BUTTONS !!! No return value when LCD_HAS_SLOW_BUTTONS is defined but not LCD_I2C_TYPE_MCP23017 !!!

Line 7) uint8_t slow_buttons; Define a local byte variable with the same name we had before but volatile uint8_t slow_buttons; !!! Can be confusing. !!!

Line 10) slow_buttons = lcd.readButtons() << B_I2C_BTN_OFFSET; read the twi register and shift out the encoder pins.

Line 11) #if ENABLED(LCD_I2C_VIKI) only for the VIKI but not for PANELOLU.

Line 12) if ((slow_buttons & (B_MI | B_RI)) && millis() < next_button_update_ms) // LCD clicked B_MI is #define B_MI (BUTTON_SELECT<<B_I2C_BTN_OFFSET) !!! BUTTON_SELECT is undefined (maybe in the library?) !!! B_RI is #define B_RI (BUTTON_RIGHT<<B_I2C_BTN_OFFSET) !!! BUTTON_RIGHT is undefined (maybe in the library?) !!! They look like binary 00010000 and 00001000 and describe the position of the pins in that register. Then B_MI an B_RI are the corrected positions when the encoder is shifted out. so (slow_buttons & (B_MI | B_RI)) is true when one of the bits for the buttons is set. millis() < next_button_update_ms most of the time, but not for the short moment between (millis() >= next_button_update_ms) and the next increase of next_button_update_ms in lcd_quick_feedback() do Line 13) slow_buttons &= ~(B_MI | B_RI); // Disable LCD clicked buttons if screen is updated erase that two bits.

Line 15) return slow_buttons; return the local variable

The remaining questions are: "What makes VIKI and PANELOLU different, so they need different handling?" and "What happens between (millis() >= next_button_update_ms) and the next increase of next_button_update_ms?"

PANELOLU2 seems to have no other buttons then the encoder. All of them can be handled fast. So no handling in slow_buttons is required at all. (http://blog.think3dprint3d.com/2013/02/panelolu2.html http://blog.think3dprint3d.com/2013/04/panelolu2-for-ramps-and-printrboard.html)

The VIKI has a lot of slow buttons, but only for two of them a function in Marlin is defined.

More to come. No more time for this today.

[AnHardt]

back again. lcd_quick_feedback() orders a screen clear and redraw, increases next_button_update_ms by 500 and makes a beep if possible. If lcd_quick_feedback() is used correctly everywhere when a button press is detected (LCD_CLICKED) , the code we are investigating is: a) a kind of debauncer for TWI buttons b) could be mend to shut down the asking for kypresses (lcd.readButtons()) during beeping und updating the screen (as the comment sugests) (lowering twi load)

In both cases the implementation is poor. Reading lcd.readButtons() all the time and then erasing the results when unwanted is not that good. Reading lcd.readButtons() only when the 500ms after a keypress are over is likely more effective.

If the b) effect is noticeable is an other thing. Asking for keypresses, beeping, updating the display are done in a sequence - they can not occur at the same time. If lcd.readButtons() was called in an interrupt before that was absolutely necessary to not corrupt twi during the beeping and updating the display. So this could be a unneeded artifact.

Waiting exited for your test results and the code that you'l produce.

EDITED

[AnHardt]

Ideas for an idle scheduler #2407

[justmyopinion]

Interesting discussion from Wurstnase. Looks like Teacup has made some good points for inspiration. Our above clarifications indicates that Marlin need some serious redesigning and suffering from far too many adhoc programming during time. But it is not a simple thing as in any realtime programming you have to consider a lot of timing from the fastest updates to the slowest like display etc and it needs a full understanding of resources used in marlin and how it could be reconfigured. maybe a discussion like starting in #2407 could be a start. But the hurdle at the moment is that dev environment has gone with the winds at the moment and before this is solved nothing creative is going to happen I presume :-(.

[Roxy-3D]

Our above clarifications indicates that Marlin need some serious redesigning and suffering from far too many adhoc programming during time. But it is not a simple thing as

But the hurdle at the moment is that dev environment has gone with the winds at the moment and before this is solved nothing creative is going to happen I presume :-(.

We are getting close to a formal and stable release now. @Wackerbarth is making changes to the file organization of Marlin and breaking things apart on the Developmental side. I think things are getting better from a Development perspective.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.