TcMenu / LiquidCrystalIO

IoAbstraction fork of Liquid Crystal Library for Arduino
http://arduino.cc/
6 stars 3 forks source link

Delay stops lcd to refresh #8

Closed stefunkk closed 3 years ago

stefunkk commented 3 years ago

Hi,

I'm a newbie in c++ & microcontrolers. But from the description i understood that it support "async".

But in this case:

taskManager.scheduleFixedRate(250, [] { // set the cursor to column 0, line 1 // (note: line 1 is the second row, since counting begins with 0): lcd.setCursor(0, 1); // print the number of seconds since reset: float secondsFraction = millis() / 1000.0F; lcd.print(secondsFraction);

});

taskManager.scheduleFixedRate(250, [] { taskManager.yieldForMicros(5000000); });

Lcd is refreshed every 5 seconds... is this how it should work? I'm looking for a solution of the issue with original LiquidCrystal_I2C and ESP8266 multitasking issue.

davetcc commented 3 years ago

If we read this line from the documentation:

All internal delays beyond initialization are now performed using taskManager's yieldForMicros()

This means that the task running the print function will not complete until the print is done, but in the meantime, the delays inside of print are handled using yieldForMicros(). This means two things for you:

  1. that other tasks can run during the pauses waiting for the hardware, as TaskManagerIO is a cooperative scheduler, based on the co-routines principle, this library gives back it's wait states by calling yieldForMicros.
  2. whenever yieldForMicros is called, the first thing it does is to call yield(), so it is very friendly to ESP boards. We use them a lot!
  3. be careful that only one task can ever call functions on the display, otherwise, the timing will be thrown off.

Don't call yieldForMicros in a task, there's no need to do that, as the runLoop does that automatically. I'll add an example shortly that demonstrates task manager integration.

the run loop (and yield for micros) essentially do:

Start Pseudocode
yield
if there has been an event or interrupt
  processEvents
end if
while taskQueue.head is not null
  if running on esp then yield
  executed first queued task
  remove task from queue 
  if task repeats add back in correct place
end while
End Pseudocode
stefunkk commented 3 years ago

Thank you for the answer but I dont get it, if i want to have 2 tasks, and one is using delay, which should i use?

Long story short ... I want to control heater with ssr, using delay in one task, and get sensor reading with display in the other one.

davetcc commented 3 years ago

Please see the new example https://github.com/davetcc/LiquidCrystalIO/blob/master/examples/TaskMgrIntegration/TaskMgrIntegration.ino

stefunkk commented 3 years ago

I have a bit different scenario. Controling the heaters mean that i have to turn it off and on for certain time based on the variable with power. So in example if i want to turn it for 100% i will be turning heater on for 2 seconds, if i want 50% i will turn it off for 1 second and on for 1 second, implementation is here: https://github.com/stefunkk/openstill/blob/master/OpenStill/HeaterTask.cpp

That's the reason why i need delay, I'm currently using esp8266Scheduler, but I can switch to any working solution

davetcc commented 3 years ago

I've added a bit more to the example, this example really belongs in task manager now! but never the less should give the general idea.

stefunkk commented 3 years ago

Thank you!

davetcc commented 3 years ago

Actually that sketch is able to reliably cause a bug where the tasks get out of order in task manager. I'll raise a critical bug in task manager now, fortunately, we have a sketch that recreates it very frequently!!

https://github.com/davetcc/TaskManagerIO/issues/15

It's actually very rare to find a bug in TaskManagerIO, I guess the base event support is quite new, there must be an edge case somewhere. It's critical so will come ahead of anything else in priority, so will be fixed soon.

stefunkk commented 3 years ago

I have problem with implementing this on my side:

https://github.com/stefunkk/openstill/tree/LiquidCrystalIO/OpenStill

in https://github.com/stefunkk/openstill/blob/LiquidCrystalIO/OpenStill/OpenStill.ino I'm registering a task:

taskManager.registerEvent(&lcdTask);

LcdTask is calling LcdService.print method:

https://github.com/stefunkk/openstill/blob/LiquidCrystalIO/OpenStill/LcdTask.cpp https://github.com/stefunkk/openstill/blob/LiquidCrystalIO/OpenStill/LcdService.cpp

In print method, when there is only Serial.println("print"); it works fine. But when i leave it as it is now it crash.

I refactor the code to make "only one task can ever call functions on the display" ... i have no idea what to do now

davetcc commented 3 years ago

according to the source, you're still using LiquidCrystal_I2C, is that what you intended? Is the latest committed?

stefunkk commented 3 years ago

No, i dont, it's a branch LiquidCrystalIO. The only presence of LiquidCrystal_I2C is commented line //LiquidCrystal_I2C lcd(LCD_ADDRESS, LCD_COLUMNS, LCD_ROWS);

davetcc commented 3 years ago

you're also passing references to things that are not global in scope, any locally declared variable is on the stack and is lost at the end of the function. You could declare these with 'new' to hold them globally as pointers, or move them to global scope.

Take a look at the service classes in your INO.

stefunkk commented 3 years ago

oops, sorry, I'm a c# developer, forgive my lack of knowledge. Now it stopped crashing, console is showing that it's working fine, but i have garbage on my lcd.

Code in repo updated

davetcc commented 3 years ago

have you called begin on the display? I ask because I don't see it in the repo version. It takes width and height.

lcd.begin(width, height);

Call it after wire is initialised.

stefunkk commented 3 years ago

Yes i did in LcdService constructor

davetcc commented 3 years ago

ok have you tried the helloI2c example to see which pin connectivity your shield has? There's two common combinations, we support wiring for both common cases: EN RW RS and RS RW EN.

stefunkk commented 3 years ago

Yes, i have example open, I tried my use case before even changing code in my repo

stefunkk commented 3 years ago

I just run your library on my previous code and it works fine

stefunkk commented 3 years ago

https://github.com/stefunkk/openstill/blob/masterWithLiquidCrystalIO/OpenStill.sln thats the branch which works fine

davetcc commented 3 years ago

Yes, it works for you because unless using task manager, it is fully blocking and there is no chance of it becoming re-entrant.

You are still calling print from more than one task as far as I can see. In your code you should only ever print to the display in the lcd service's exec method. It is an important distinction, LiquidCrystalIO does allow some degrees of freedom from delays, but as always with asynchronous code, it brings restrictions on how it can be used. In this case, all operations with the display must be either within a single task's function or exec() method.

stefunkk commented 3 years ago

that's exactly what I'm doing, only LcdTask is able to print

stefunkk commented 3 years ago

btw. other tasks are commented out, nothing else is running

davetcc commented 3 years ago

I'll be honest, trying to change scheduling libraries mid way through a project is a bad idea, I think you should stick with what you had in all honesty. Especially given that you've got a 90% solution, I recommend that you switch to a small cheap OLED screen and you won't have this problem.

I didn't realise that you had already an almost complete solution until mid way through the conversation. You can get an SPI SSD1306 128x32 for way less than $10, that updates in a couple of milliseconds. You will not see noticable delays with that solution.

stefunkk commented 3 years ago

It's a beginning of the project. I can't use old ones because the lcd is refreshed once every 5 seconds. To make it even strange Serial print shows that the code with lcd.print is executed very often, but value on device is not refreshed. Is display the issue or esp8266 device, switching to esp32 wont solve the problem?

stefunkk commented 3 years ago

So ... I've made a example with a bug, in single file:

https://pastebin.com/BUcQ2vni

davetcc commented 3 years ago

I've tested it on an Uno and on a Nano 33 BLE, with a 16x2 Df robot LCD shield and an i2c 20x4. It worked on both without incident. I had to change the address to 0x20 for my i2c display, and I replaced println with print. Other than that no changes. I'll see if I can free up an esp8266 and i2c display over the weekend to test it with the same hardware.

stefunkk commented 3 years ago

Hold on, i believe that the code is working on my side as well, i need a break, i'm fighting with this issue couple days already.

davetcc commented 3 years ago

OK, I recommend that you take a step back then verify everything. What you're trying to do is a big change, and it may take you quite a bit of time to get it working properly. This is not C# development, that's for sure.

stefunkk commented 3 years ago

Oh God, I found it. So, my sensorTask, which gather the data from ds18 sensors have property with object SensorData, which have array with readings. I'm updating those readings in sensorTask:

    for (uint8_t* element : _addresses) {
        auto temp = _sensors.getTempC(element);
        _sensorData.temperatures[counter] = temp;
        counter++;
    }

In LcdTask which also have pointer to same sensorData object, I'm passing sensorData array to function which print the results.

In this way, I'm getting alot of moving garbage on the screen.

But when I just add Serial.println(temp), like that:

    for (uint8_t* element : _addresses) {
        auto temp = _sensors.getTempC(element);
        _sensorData.temperatures[counter] = temp;
        Serial.println(temp);
        counter++;
    }

Everything works fine, lcd display correct result. I'm totally lost. I found it by accident while trying to found a place on which it breaks.

edit...

I had to move all the includes and initialization to the class LcdService, passing it from main doesnt work.

stefunkk commented 3 years ago

I found out that in the past, before writing code for display i put heater RELAY_PIN as D2, which on esp8266 is the pin to which you connect i2c devices. It cost me i don't even know how many hours. Thank you for you time, and valuable tips.