Hi C,
Many thanks for the reply.
Oh believe me I have no more Skill than the next person.
Just quite a lot of free time and little distraction.
I looked over the code I posted and there is much room for improvement,
so don't go thinking you are done.!!
Sorry for the extra unknown stuff in the example.
With little projects that require two versions of similar code I find it easier sometimes to wrap the two halfs in the same projects as many of the strings, commands and structure relate.
Blocking Code.
My take on this is you should never stay in one place for too long without giving some control back to the background code to let it do its house keeping. Stuff like the serial, wifi, timers and other background tasks all need some time to do their thing.
There is also the need to check things have actually started or exist before trying to read or manipulate.
Like Cattle... mentioned if(client.available()){//it exists}
Also calling something that is going to wait for a certain thing to arrive.
Not sure if the readstringuntil has any sort of timeout or if it will just sit there waiting.
But it is always best to ask "does the room have a door" before you try and find a way out.
Things like Delay() & yield()are great as they do some house keeping thru the process,
Using flags and timers to give a delay can be less intrusive than delay() and allow the background tasks a bit of air to do their thing.
bool alreadyhere = false;
void mydelay(int dly){
unsigned long starttime = millis();
while (1==1){
refresh(false);
if ((millis()-starttime) >= dly){break;}
}
}
void refresh(bool full){
yield();
WIFI_loop();
if (alreadyhere){return;}
alreadyhere = true;
// perhaps some other loops() that may also use mydelay().
LCD_loop();
alreadyhere = false;
}
void WIFI_loop() {
C_webSocket.loop();
S_webSocket.loop();
ESP_server.handleClient();
}
You just have to make sure that you do not crash the stack by not using mydelay() in anything that could be called as a result of the refresh() call.
Not sure if that makes sense, or even if it is accepted practice but it works for me.!
And this was the (bad) example of using timers and flags to resist the delay().
If you wanted to only do something once per second.
//unsigned long askTimer;
void loop(){
server.loop();
if((millis() - askTimer) < 1000){return;}
askTimer = millis();
// do something
}
is slightly better than
void loop(){
server.loop();
// do something
delay(1000);
}
Debounce:
Basicly switches can often give multiple high and lows during the process of activation.
It is good practice to read the state of switch inputs for a minimum period of time to establish a positive activation rather than just a glitch.
In your case it is not really needed as you have a long process happening if the button is pressed before you then test if the button is not pressed. so little chance of multiple triggers.
Others will describe that better perhaps.
Oh and a BIG issue is the checking of the alarm time.
It is limited to an exact match lasting for only 1 second.
This could easily be missed if the device was busy elsewhere.
Best to perhaps have a slightly more complex process of detection, timers and flags so you do not miss that appointment.
Perhaps use timeserial rather than the hours:mins:secs.
That will give an easier way to say
if ( (ntp.timeserial >= alarmtime-1000) && (ntp.timeserial <= alarmtime+1000) ){
if (donealarm == false){
donealarm = true;
// Start the alarm process
}
} else {
donealarm = false;
}
Sorry the names (or numbers) may not be right.
Also consider using a softRTC along side your NTP solution, so that you do not need to call the update on every loop perhaps.
You could call the time.update at start once connected and then on the hour or every 30mins perhaps. That update call to the timeserver may be the reason for the slightly odd tones you mention. But to be honest I have not looked at the NTP code so not sure what it gets up to.
Sorry got to dash out, will finish off later