search

universam1 / iSpindel

electronic Hydrometer
http://www.ispindel.de
Other
827 stars 322 forks source link

iSpindel zero values #508

Closed BernhardSchlegel closed 3 years ago

BernhardSchlegel commented 3 years ago

I'm submitting a ...

Did you follow the general troubleshooting steps first:

Report

According to the log, iSpindle reports a "Zero" sporadically.

iSpindle_Zero

Is this a known Issue?

Console Logs

I don't have any as of now: recording logs over a long time is difficult + problem became visible during first fermentation and did not surface in offboard test. iSpindle reports the gravity every 15 minutes.

Steps to Reproduce (for bugs)

Code used see here, I've added only a new transmission possibility (PR request incoming).

  1. Setup iSpindle
  2. track fermentation
pppedrillo commented 3 years ago

What does it say in log when readings are zeroes?

16:39:27.969 -> Samples:30 min:90.14 max:91.02 time:2193

BernhardSchlegel commented 3 years ago

Thanks for the reply ! My iSpindle is currently floating in beer ;) After fermentation is done, I will try to reproduce the error outside the fermenter and update this bug report with the log.

pppedrillo commented 3 years ago

May be you can capture raw incoming data on your server just to check how json looks like when tilt value fails? Is it only gravity value is 0 or all the others as well?

BernhardSchlegel commented 3 years ago

I tried, but (idk why), despite I added those info to the sender json

sender.add("s_number_voltage_0", Volt);
sender.add("s_number_wifi_0", WiFi.RSSI());
sender.add("s_number_tilt_0", Tilt);

they do not end up in the stringified json

String json;
serializeJson(_doc, json);

server only receives the following

{"apikey":"aaaa123","type":"ispindel","brand":"wemos_d1_mini","version":"6.6.0","chipid":"1458415_5C:CF:7F:D0:83:EE","s_number_wort_0":5.095389,"s_number_temp_0":18.25}
pppedrillo commented 3 years ago

May be kinda packet length limitation or likewise?

And be careful with "chipid":"1458415_5C:CF:7F:D0:83:EE" We are under GDPR in EU https://en.wikipedia.org/wiki/General_Data_Protection_Regulation It is better to remove it from the json. Otherwise, I believe, you have to put GDPR discalmer onto whole the project.

BernhardSchlegel commented 3 years ago

Thought so, too. But _doc is defined in Sender.h as StaticJsonDocument<1024> _doc;. Two reasons speak against the length limitation: 1. current payload is much shorter (ususally around 170 bytes / chars). 2. Payload is never "cut-off". It's always terminated by a clean }. BUT the fact that changing the sender.add() order changes which fields are cutoff speak FOR your theory. I don't have a clue (yet).

Thanks for pointing out the GDPR. I have a GDPR disclaimer on the project side (receiving wise). But I'd be fine with hashing the chip ID - it just has to be unique and permanent (cause one BierBot Bricks Brewery might have multiple iSpindels). Do you think having the GDPR disclaimer backend-wise is sufficient? Do you think it would be ok to use ArduinoMD5 memory footprint-wise?

lbussy commented 3 years ago

Payload is never "cut-off". It's always terminated by a clean }.

An ArduinoJson doc is always defined as {}. As you add items to it, the memory (1024 in this case) will be debited. Once you have exceeded that memory, additional keys and values will not be added, and no exception is raised. If you are missing items, increase the allocated amount and you should get them back.

I'm not saying this is your issue, but I am saying that "clean" JSON will always be formed.

There is an excellent calculator on the website.

BernhardSchlegel commented 3 years ago

Very interesting, thanks @lbussy ! But I didn't have the balls to increase the global _doc buffer size. However (what's strange): Other "transmission methods" use way more fields than what's required by Bricks - but I did not check if anything is really transmitted there.

I ran the calculator using the provided payload above. I'd interprete that as "it shoud work"?

image

pppedrillo commented 3 years ago

Someone already had a balls ;) pls see commit # f4d98cbe699db79f5aa5fa30aeb51d68e9d13ef6 256->1024

lbussy commented 3 years ago

I agree with your calculations - I just didn't want you to think that because it was valid JSON that it was still not truncated. I've definitely been there, did that.

There are a few to help debug this.

BernhardSchlegel commented 3 years ago

Thanks @pppedrillo for pointing that out. I'll update my iSpindel and look for the tilt reading backend-wise. Will update this Issue afterwards.

Thanks @lbussy : That's plenty of really good intel :) Thanks-a-lot! Especially the JsonDocument::overflowed() seems very interesting to me. Maybe even to output errors on a Global level?

pppedrillo commented 3 years ago

Do you think having the GDPR disclaimer backend-wise is sufficient?

AFAIK any thing which will spy should have it. It this case if device spies - it has to have. And backend as well of course if it process the data.

Do you think it would be ok to use ArduinoMD5 memory footprint-wise?

guess it should be fine.

BernhardSchlegel commented 3 years ago

I'll update my PR tonight. Since ESP.getChipId() is used by other protocols (API_GENERIC, API_FHEM) as well, do you think it make's sense to add the hashing there as well in one go?

pppedrillo commented 3 years ago

Another wild guess: what the length of the real API key? "apikey":"aaaa123"

my_token is only [TKIDSIZE * 2]

BernhardSchlegel commented 3 years ago

apikey is only 20 chars long

pppedrillo commented 3 years ago

do you think it make's sense to add the hashing there as well in one go?

Will not hurt, I see no major issues except current users of these APIs might need to update (perhaps) that ID if it is using somehow on platform in a special way.

@universam1 comment may be?

pppedrillo commented 3 years ago

apikey is only 20 chars long

Hmm, all good then there....

I think I'll build your changes and give it a shot. I let you know if 'll find anything.

BernhardSchlegel commented 3 years ago

do you think it make's sense to add the hashing there as well in one go?

Will not hurt, I see no major issues except current users of these APIs might need to update (perhaps) that ID if it is using somehow on platform in a special way.

@universam1 comment may be?

hashed chipid for my backend, see this commit. Encapsulated it into a method (without additional libs), can be easily applied to other backends. IMHO: ESP.getFlashChipId() is not guaranteed to be unique, thus probably not GDPR relevant.

pppedrillo commented 3 years ago

@BernhardSchlegel Looks good, I already picked up the changes and see chipid well hash-looked

pppedrillo commented 3 years ago

@BernhardSchlegel Well, I played with your pull request changes for some time and here are my observations:

  1. With changes as they now I constantly and almost 100% have a crash during auto httpCode = http.POST(json); call
  2. Same story if instead of local WiFiClientSecure client; I use _secureClient (the member of Sender class)
  3. Same story if I strip away setInsecure() and connect(url, 443);, thus just pass the ref to http.begin() without any pre configs

These crashes hurt iSpindel so bad, that it often gets back into configuration mode after restart.

  1. Now the good news. No crashes if I use _client (the member of Sender class). Obviously it can't send anything (because not configured), but does not crash and http.POST(json); returns HTTPC_ERROR_CONNECTION_LOST after a short timeout.

Just my 2 cents, I hope it helps :)

BernhardSchlegel commented 3 years ago

@pppedrillo thanks for looking into it! And thanks for your patience. I've moved my pull request back to "draft" this afternoon since my iSpindel is currently in a fermenter and I have no chance to really try out my code until Sunday. However, the original code as of the creation of my PR is running just fine for one week :)

Bear with me until Sunday, I'll figure it out and fix it!

pppedrillo commented 3 years ago

@BernhardSchlegel Sounds good.

original code as of the creation of my PR is running just fine for one week

It might be because you have a real token/API key. I used "1234" or likewise :) Not sure if this caused a crash but still.

In my message I also put "almost" word before "100%". By this I meant it is crashing often but not every single post. Then it survives, it returns "500" and this can be because of wrong/missing token.

My point - its better to see a logs on device. Because "running just fine for one week" may in reality looks like "crash-crash-crash-send-crash-send-send-send-crash.... etc etc" :)

BernhardSchlegel commented 3 years ago

Hi,

it indeed crashes - every single time (on http.POST())

I coded the exception dump, extract:

0x40234d81: operator delete(void*) at /workdir/repo/gcc/libstdc++-v3/libsupc++/del_op.cc:48
0x401004ee: millis at ??:?
0x4021ff8e: optimistic_yield at ??:?
0x4020e52b: BearSSL::WiFiClientSecure::_run_until(unsigned int, bool) at ??:?
[...]

Note the first line. Crash on freeing memory - points IMHO towards memory issue

So I went on a time travel and compared the last commit (that ran for two weeks now on my iSpindle) to the next (merge the master chances, e.g. littleFS).

Comparison here

Only differences code-wise that could have effect:

  1. StaticJsonDocument<256> _doc; became StaticJsonDocument<1024> _doc;
  2. new JavaScript field "hassio":0

I fixed 2. and after decreasing the StaticJsonDocument-size to 512 it worked again. Makes sense to me, since there is nothing crazy in my code. Because of the latter: I can hardly believe that "my" service (Bricks) is the only one affected.

Can we agree on lowering StaticJsonDocument buffer to 512? (which is btw enough to transmit all 9 fields of the bricks backend, which supports @lbussy s' statement).

Thanks!

pppedrillo commented 3 years ago

Can we agree on lowering StaticJsonDocument buffer to 512?

Changes 256->1024 were done in commit # f4d98cbe699db79f5aa5fa30aeb51d68e9d13ef6 it has a comment "Change sender _doc size to 1024 so all items fit when sending TCP". If turning size back to 512 will not cause losing of data in other than Bricks case, it would be ok, but I'd rather look more at the real reason of crash. I am almost 100% sure that http.POST along is able to send more than 1k data.

BernhardSchlegel commented 3 years ago

Hi @pppedrillo , I went thoroughly through my code and was not able to find anything suspicious (nothing fancy there, that is not already somewhere else in the iSpindle code). So I did the following experiment.

Setup

Select Service Type=HTTPS Post, Server Adress=https://httpbin.org/, Path / URI=post

Experiment

With StaticJsonDocument<512> _doc (Firmware as of my PR): works fine. :heavy_check_mark: Changed to StaticJsonDocument<1024> _doc: Crashes every time. :heavy_multiplication_x: changed back to StaticJsonDocument<512> _doc: works fine - every time. :heavy_check_mark:

Result

My conclusion would be 1024 is bad, at least for HTTPs Posts (which affects the already existing HTTPS Post service option as well as my new Bricks option) - and presumably also other options. Thus: I'd strongly vote for 512. If you want to look at my code, I'm happy to offer a free BierBot Brick including shipping if you happen to spot a bug :)

Let me know what you think! maybe we should continue this discussion in the thread of my PR?

Cheers

richow commented 3 years ago

Comrades, I have also had a 0sg readings from my iSpindel with all other readings being present. Yet the configuration screen was showing a valid SG reading. I am using the HTTP method sending to a local server. Saving the raw JSON simply showed a 0 for the gravity. The good & bad news is after I updated from v7.1.0 to v7.1.2 the problem has gone away. Good because it's working. Bad because I can't see why.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.