itead / ITEADLIB_Arduino_Nextion

MIT License
329 stars 318 forks source link

NexHardware.cpp Major Code Design Flaw Causing Missed User Input!! #31

Open CircuitSerialKiller opened 7 years ago

CircuitSerialKiller commented 7 years ago

Hello, If you implement this code in a device where you are using a microcontroller to refresh the Nextion screen with new data periodically, as well as allow user touch input, user input ends up often being ignored because of this way this code is written, especially when the microcontroller is busy doing something else when the input is sent from the Nextion to the microcontroller. As it stands, you have to call nexLoop before sending any command to the Nextion to make sure the serial buffer isn't cleared out from the code below, which in itself could create all sorts of user interaction timing issue. Also, if you were to eliminate clearing the serial buffer and send a command without calling nexLoop before sending the command, each of the three methods below that also read from the Serial buffer will also cause a problem since there will be user touch input queued in the serial buffer, not the response! bool recvRetNumber(uint32_t number, uint32_t timeout) uint16_t recvRetString(char buffer, uint16_t len, uint32_t timeout) bool recvRetCommandFinished(uint32_t timeout)

Take a look at the beginning of the below method. Why would you clear out the serial buffer from queued commands? void sendCommand(const char* cmd) { while (nexSerial.available()) { nexSerial.read(); }

There should be only one method doing nexSerial.read() in NexHardware.cpp. It should read the serial buffer stack, and based on the byte code header sent from the Nextion, call the appropriate method based on the data received from the Nextion after receiving a full command.

Please contact me if you need assistance as I'm already starting a full rewrite.

Clearly the only issue with a proper implementation is that you may want to eliminate confirmation of commands to the Nextion as there's no way to really implement this without queuing the user input. Leave it up to the use to re-read the value sent to the Nextion to validate that it is set properly.

Tags: Nextion Arduino Library Intermittently Missing Button Presses and User Input

CircuitSerialKiller commented 7 years ago

Futhermore, everything about this code is flawed. In NexLoop, you added delay(10); to wait for additional data after NEX_RET_EVENT_TOUCH_HEAD is received.

You should be queuing all input into a circular ring buffer every time nexLoop is called. Once you have a valid complete received command from the Nextion at the front of the queue, then call then iterate.

palivoda commented 7 years ago

Ah... reported same issue https://github.com/itead/ITEADLIB_Arduino_Nextion/issues/33 and then found yours.

CircuitSerialKiller commented 7 years ago

I just thumbed up your post. To get around this for now, effectively bypassing the validation check (and will only work if you don't need to rx numbers or strings back, in NexHardware.cpp I just added: return true; As the first command in the following methods: bool recvRetNumber(uint32_t number, uint32_t timeout) uint16_t recvRetString(char buffer, uint16_t len, uint32_t timeout) bool recvRetCommandFinished(uint32_t timeout)

I also commented out the following code in sendCommand(const char* cmd)

{
    nexSerial.read();
}
palivoda commented 7 years ago

Thanks! "return true" wont work for me. I'm actively sending numbers from Nextion and catch in Arduino with recvRetNumber.

Due to memory consumption issues I've ripped out serial communication functions from this library and use only them. I dont use native event handler, instead I just send click events with prinh, like

printh 65 01 01 00 ff ff ff also you can values from Nextion like get VARIABLE.val for sending I still did not found hot to make is shorter than sending "full variable name = value". Lack of string parsing functions. I wish I send all values in one string and parse it on Nextion side

CircuitSerialKiller commented 7 years ago

You're right - I just updated my comment. I'm not receiving text or numbers from the display - just sending them to the display, so bypassing validation works for me. I actually went that route because of the missed touch input - I needed to guarantee that what was on the display was actually the values in the microcontroller. I press a button on the display to increment or decrement a value, then send that command to the microcontroller, then have the microcontroller update the display to reflect that it actually received the command and the value was set properly. I'm using the native event handlers for now, but sending commands directly to the Nextion. I also upped the Nextion serial speed to 115200.

Bascy commented 7 years ago

Running exactly into the same problem .. have written my own event handler for receiving commands only to discover that everything goes haywire when handling the event could result in a getvalue or setvalue type of operation sending to the nextion and waiting for a result ...

I am very interested in any wel designed solution for this!

alexborro commented 7 years ago

I don't believe they wrote such code.. I was looking for some issue in serial interrupt, etc... Never thought such noob design flaw til I opened the nexloop() code.

pramodmadhav commented 7 years ago

instead of using Serial.available() which is a polling type, in this case, one should use interrupt driven Serial communication like in NeoHWSerial which is available on git(https://github.com/SlashDevin/NeoHWSerial) @CircuitSerialKiller

Viljams commented 7 years ago

Hi! Do I have the same problem described here in this thread? I created project using Nextion LCD and Arduino. I created several pages and navigation buttons using nex.poll(); and those nextion event callbacks. Navigation trough my pages worked fine and flawlessy until I used nex.getCurrentPage(); function for arduino to know which page is open. and case-switch statement for each page. Now I have to press sometimes twice to ten times on button to get some input(open next page) I fixed this by increasing delay time at the end of my get-currentpage-case-swich statment. When I added some code to update page content-text/labels RTC clock for example. Problem is back. Navigation don't work normally. Don't know what to do :(

pramodmadhav commented 7 years ago

my simple workaround is while using the lib do not use nexloop(),nexlistenlist,and attachpopor push, use simply Serial.available(); Serial.read in conjunction with print or printh

DrStein99 commented 7 years ago

I converted my files to use Stream() object instead of "serial" or softwareserial. This way I can pass hardware serial, softwareserial, altsoftserial or ANY stream object - and use that.

Also - in case actually ONE other person out there has MORE THAN ONE display and MORE THAN ONE project that uses a Nextion display, you won't need to modify your library files to specifically designate which serial object you only want to use.

alexborro commented 7 years ago

That's pretty good. Where can we find your lib?

Em 13 de jul de 2017 4:59 AM, "DrStein99" notifications@github.com escreveu:

I converted my files to use Stream() object instead of "serial" or softwareserial. This way I can pass hardware serial, softwareserial, altsoftserial or ANY stream object - and use that.

Also - in case actually ONE other person out there has MORE THAN ONE display and MORE THAN ONE project that uses a Nextion display, you won't need to modify your library files to specifically designate which serial object you only want to use.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/itead/ITEADLIB_Arduino_Nextion/issues/31#issuecomment-315002335, or mute the thread https://github.com/notifications/unsubscribe-auth/AE-ZEzDucwQTVe9VbqYS2dDvEDrCzmLsks5sNc56gaJpZM4LVE6G .

Bascy commented 7 years ago

We have switched to NeoNextion lib. A much higher quality library (actually looks like there was spent some time on designing the code) and it also uses a Stream as the communication channel

DrStein99 commented 7 years ago

I do not have a heavily modified library with tidy code. I make corrections, test to work and only have time to get it to run. It's not a magor re-write, and still tripping over "receive error" trying to figure out why function wasnt made to queue return callbacks from a stack.

The NeoNextion library looks nice, I spent about 1/2 hour exploring the code. I will probably try that one out next, since it actually had updates within the whole last year.

alexborro commented 7 years ago

The source of problem is the Nextion protocol. The answer doesn't have the executed command, so you cannot have an asynchronous communication... I mean, you must send a command and wait for the answer.. It will be a lot better if the host could receive the answers in any order, parser them and process the response.

But we have to live with it as is.

Cheers.

Alex

2017-07-13 17:53 GMT-03:00 DrStein99 notifications@github.com:

I do not have a heavily modified library with tidy code. I make corrections, test to work and only have time to get it to run. It's not a magor re-write, and still tripping over "receive error" trying to figure out why function wasnt made to queue return callbacks from a stack.

The NeoNextion library looks nice, I spent about 1/2 hour exploring the code. I will probably try that one out next, since it actually had updates within the whole last year.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/itead/ITEADLIB_Arduino_Nextion/issues/31#issuecomment-315199064, or mute the thread https://github.com/notifications/unsubscribe-auth/AE-ZE1QmU_bcq1dLHjyWOhDXEUFQDzFAks5sNoPQgaJpZM4LVE6G .

-- "Não é o mais forte da espécie que sobrevive, nem o mais inteligente. É aquele que se adapta melhor as mudanças" ( Charles Darwin )

Alex Borro

DrStein99 commented 7 years ago

So, I should probably just ditch the receive reply - since it is so unreliable, and only presents a burden to the rest of the execution on the program. Functions that are critical to demand a reply from sent data, I guess we need to get creative and figure something out.

I guess if there was a way to code the Nextion firmware, it would probably be somewhere in my searches.

bspranger commented 7 years ago

I had a long time ago modified the library to send all commands from the display to the arduinom. So if a value changed I sent a command from the display to notify the arduinom of the change AND the value.

This ment my arduino never had to poll the display for data. It worked great. I committed it to my branch and made a poll request but it was all disregarded by mention.

The code is still there and I haven't touched it since so it does not support any of these new objects, but from my experience, it was easy to integrate.

DrStein99 commented 7 years ago

The Nextion firmware is kind of sensitive, since it has to interface with that wonderful GUI desktop designer application, to update the .hmi project.

Where did you find how to modify the Nextion firmware library for that device?

bspranger commented 7 years ago

I didn't have to edit the display firmware, I was able to make my own commands in the GUI.

But I found that once the communication was only one direction I never had issues with missing events, getting out of sync etc.