Support SG on BierBot Bricks #559

BernhardSchlegel · 2021-12-13T06:59:45Z

If iSpindel transmits SG (and not °P) 1 decimal (1.0, round(x*10)/10) is not sufficient. This PR ensures, that 4 decimals are send to the BierBot Bricks backend. This way, gravity values like SG=1.0482 can be transmitted to the BierBot Bricks backend without loss of information.

If iSpindel transmits SG (and not °P) 1 decimal (1.0, `round(x*10)/10`) is not sufficient.

jonathanschneider

Commit changes decimal point of gravity measurement sent to BierBot bricks to support SG and has no effect on the rest of the firmware.

pppedrillo · 2021-12-27T10:11:42Z

4 decimals in SG can be a bit of overshoot.

BernhardSchlegel · 2022-01-08T09:12:32Z

That's one more than actually needed to be able to round backend-wise and enable data freshness checks. I'm happy to change it if really necessary. But IMHO it should not hinder the merge.

WireTownMan

This change corrects a defect that allows bierbots to use Plato or SG. Does not impact anything other than number of decimal places transmitted

thegreatgunbantoad · 2022-01-09T04:23:16Z

API_BRICKS seems to be the only API that does this. It this truncation needed at all?

BernhardSchlegel · 2022-01-09T08:43:21Z

I'm personally a fan of a deterministic memory footprint in non-garbage collected environments and deterministic POST payloads. But: I don't want to force other APIs to follow this approach.

Is a discussion about one decimal more or less really worth our time? I would really appreciate if we come to an agreement :) If that's the only concern hindering the merge, I'm happy to change it to avoid further waste of time and energy on all sides.

pppedrillo · 2022-01-09T11:22:51Z

Is a discussion about one decimal more or less really worth our time? I would really appreciate if we come to an agreement

This discussion at least can help to save this pull request from the stale bot's shredder :) Apparently, you haven't looked at these things from this point of view :)

BernhardSchlegel · 2022-01-09T13:02:18Z

hehe, that's indeed correct. So how do we proceed? Can someone with the necessary rights to merge a PR make a clear statement ("remove truncation"/ "truncation to three decimals is fine" / "trundcation to four decimals is fine")?

Thanks in advance!

WireTownMan · 2022-01-09T17:12:15Z

Is a discussion about one decimal more or less really worth our time? I would really appreciate if we come to an agreement

This discussion at least can help to save this pull request from the stale bot's shredder :) Apparently, you haven't looked at these things from this point of view :)

It is all about decimal places. With 1 decimal place bierbots will only support Plato units, with 3 or more decimal places it will support Plato and Specific Gravity (SG). So, yes this discussion is all about one the number of decimal places transmitted to the bierbots API

pppedrillo · 2022-01-09T17:41:37Z

It is all about decimal places. With 1 decimal place bierbots will only support Plato units, with 3 or more decimal places it will support Plato and Specific Gravity (SG). So, yes this discussion is all about one the number of decimal places transmitted to the bierbots API

But still 4 decimals doesnt make any sense because you simply dont have an instrument to measure with such precision (I believe you need a device that can measure gravity up to 5 decimals to get reliable 4 decimals result).

Overall, to me it sounds like an attempt to fix bierbots issue (eg unable to configure gravity units in bierbots) on the iSpindel side? Why dont fix it on bierbots side?

WireTownMan · 2022-01-09T17:58:20Z

It is all about decimal places. With 1 decimal place bierbots will only support Plato units, with 3 or more decimal places it will support Plato and Specific Gravity (SG). So, yes this discussion is all about one the number of decimal places transmitted to the bierbots API

To me it sounds like an attempt to fix bierbots issue (eg unable to configure gravity units in bierbots) on the iSpindel side? Why dont fix it on bierbots side?

Fixing on the Bierbots side would force ispindel to be a device that is setup for Plato units. This is inconsistent with other interfaces that allow the gravity to be either P or SG.

BernhardSchlegel · 2022-01-09T18:35:10Z

Overall, to me it sounds like an attempt to fix bierbots issue (eg unable to configure gravity units in bierbots) on the iSpindel side? Why dont fix it on bierbots side?

Nope, BierBot Bricks backend can't do much if the iSpindle user decided to go with SG and sends a 1.0 reading...

But again: I'm happy to fullfill any request to be able to merge. Please just tell me what you want.

pppedrillo · 2022-01-09T20:57:47Z

@BernhardSchlegel
Sorry I feel I still missing something (sorry, I'm not familiar with bierbots API).
Putting aside deterministic things, what if there would be no truncation at all?
I mean from the very beginning - in your very first bierbots support version. If all the data which currently truncated (SG, Temp, Voltage and tilt) would be sent as is (eg their "raw" values), as all the other APIs do?
If truncation (or any other conversion) need, it must be done on the bierbots side?
And no need to put a magic numbers 10, 10000 etc and change them to "fix" something later?

thegreatgunbantoad · 2022-01-10T01:02:05Z

I think it may be worth splitting this into two items. One quick and easy fix as this current request has. And then maybe open a discussion point as to whether or not the truncation can be removed altogether.
From what I can see the current correction looks fine, and I don't see a reason not to merge it.
Long term I do think we need to answer whether or not the raw value could be used.

thegreatgunbantoad

This looks fine, but please open a discussion point to raise the issue of whether or not BierBot Bricks could simply accept the raw untruncated value.

pppedrillo · 2022-01-10T07:53:24Z

From what I can see the current correction looks fine, and I don't see a reason not to merge it.

This merge can potentially cause future "fixes" in the same style.
The problem was with initial implementation which had such truncation from the beginning.

If there is no technical showstoppers for sending raw values without truncation, I'd propose change values to raw now and forget about bierbots fixing for a while. @thegreatgunbantoad

pppedrillo

I'd propose to get rid of truncation and send a raw values in the same way as the other APIs do.

pppedrillo · 2022-01-10T07:55:53Z

pio/src/iSpindel.cpp

@@ -788,7 +788,7 @@ bool uploadData(uint8_t service)
    sender.add("brand", "wemos_d1_mini");
    sender.add("version", FIRMWAREVERSION);
    sender.add("chipid", chipidHashed);
-    sender.add("s_number_wort_0", (float)(round(Gravity * 10) / 10));
+    sender.add("s_number_wort_0", (float)(round(Gravity * 10000) / 10000));


Can we consider to send a raw values (without truncation) instead of such fixes with changing a magic numbers?

pppedrillo

LGTM @BernhardSchlegel
Clang format has to be run onto iSpindel.cpp to get rid of lint errors. Format Document or Shift+Alt+F in VC Code

pppedrillo

Can be merged as soon as build gets fixed,

EDIT: fix #566

thegreatgunbantoad · 2022-01-10T18:59:12Z

From what I can see the current correction looks fine, and I don't see a reason not to merge it.

This merge can potentially cause future "fixes" in the same style. The problem was with initial implementation which had such truncation from the beginning.

If there is no technical showstoppers for sending raw values without truncation, I'd propose change values to raw now and forget about bierbots fixing for a while. @thegreatgunbantoad

The sort of point I was going for was that this was a fix in the style of the original code which I can't see any major issue with.

Secondary to that was the query of weather the original should have been coded that way which is a bit of a different issue and could have been made it's own pull request in order to keep the two issues segregated.

pppedrillo · 2022-01-10T19:10:06Z

Secondary to that was the query of weather the original should have been coded that way which is a bit of a different issue and could have been made it's own pull request in order to keep the two issues segregated.

Looks like this is not "two issues" (which should be kept segrregated) but the only one and the same. Nevertheless, @BernhardSchlegel already removed truncation. @thegreatgunbantoad

thegreatgunbantoad · 2022-01-10T19:16:28Z

Secondary to that was the query of weather the original should have been coded that way which is a bit of a different issue and could have been made it's own pull request in order to keep the two issues segregated.

Martin, looks like this is not "two issues" (which should be kept segrregated) but the only one and the same. Nevertheless, @BernhardSchlegel already removed truncation. @thegreatgunbantoad

Indeed. I still don't think they needed to be the same problem (solve one thing at at time or you'll never get anywhere), but one did certainly raise the other. But that looks moot now anyway. Cheers to @BernhardSchlegel for not losing his mind while we all discuss the awkwardness.

thegreatgunbantoad · 2022-01-10T19:25:49Z

@pppedrillo huh, didn't realise git hub knew my name.

thegreatgunbantoad · 2022-01-10T19:59:49Z

@BernhardSchlegel, oh cool are you the BierBot-Bricks developer, didn't realise that. That's cool.

BernhardSchlegel · 2022-02-04T15:58:17Z

Can we please merge this?

universam1

👍🏻

BernhardSchlegel · 2022-02-04T16:05:18Z

Thanks!

BernhardSchlegel added 2 commits December 13, 2021 07:55

support SG on BierBot Bricks

29b2390

If iSpindel transmits SG (and not °P) 1 decimal (1.0, `round(x*10)/10`) is not sufficient.

updated to 4 decimals (SG=1.0482)

7a5756b

jonathanschneider previously approved these changes Dec 13, 2021

View reviewed changes

WireTownMan previously approved these changes Jan 8, 2022

View reviewed changes

thegreatgunbantoad previously approved these changes Jan 10, 2022

View reviewed changes

pppedrillo suggested changes Jan 10, 2022

View reviewed changes

remove value truncation

bd2b898

BernhardSchlegel dismissed stale reviews from thegreatgunbantoad, WireTownMan, and jonathanschneider via bd2b898 January 10, 2022 09:08

fixing docu

2ab9e9f

pppedrillo previously approved these changes Jan 10, 2022

View reviewed changes

clang format comment

3398e86

BernhardSchlegel dismissed pppedrillo’s stale review via 3398e86 January 10, 2022 10:04

pppedrillo approved these changes Jan 10, 2022

View reviewed changes

universam1 approved these changes Feb 4, 2022

View reviewed changes

universam1 merged commit a66ba1d into universam1:master Feb 4, 2022

BernhardSchlegel deleted the patch-1 branch February 4, 2022 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SG on BierBot Bricks #559

Support SG on BierBot Bricks #559

BernhardSchlegel commented Dec 13, 2021

jonathanschneider left a comment

pppedrillo commented Dec 27, 2021

BernhardSchlegel commented Jan 8, 2022

WireTownMan left a comment

thegreatgunbantoad commented Jan 9, 2022

BernhardSchlegel commented Jan 9, 2022

pppedrillo commented Jan 9, 2022

BernhardSchlegel commented Jan 9, 2022

WireTownMan commented Jan 9, 2022

pppedrillo commented Jan 9, 2022 •

edited

Loading

WireTownMan commented Jan 9, 2022

BernhardSchlegel commented Jan 9, 2022 •

edited

Loading

pppedrillo commented Jan 9, 2022 •

edited

Loading

thegreatgunbantoad commented Jan 10, 2022

thegreatgunbantoad left a comment

pppedrillo commented Jan 10, 2022 •

edited

Loading

pppedrillo left a comment

pppedrillo Jan 10, 2022

BernhardSchlegel Jan 10, 2022

pppedrillo left a comment •

edited

Loading

pppedrillo left a comment •

edited

Loading

thegreatgunbantoad commented Jan 10, 2022

pppedrillo commented Jan 10, 2022 •

edited

Loading

thegreatgunbantoad commented Jan 10, 2022 •

edited

Loading

thegreatgunbantoad commented Jan 10, 2022

thegreatgunbantoad commented Jan 10, 2022

BernhardSchlegel commented Feb 4, 2022

universam1 left a comment

BernhardSchlegel commented Feb 4, 2022

Support SG on BierBot Bricks #559

Support SG on BierBot Bricks #559

Conversation

BernhardSchlegel commented Dec 13, 2021

jonathanschneider left a comment

Choose a reason for hiding this comment

pppedrillo commented Dec 27, 2021

BernhardSchlegel commented Jan 8, 2022

WireTownMan left a comment

Choose a reason for hiding this comment

thegreatgunbantoad commented Jan 9, 2022

BernhardSchlegel commented Jan 9, 2022

pppedrillo commented Jan 9, 2022

BernhardSchlegel commented Jan 9, 2022

WireTownMan commented Jan 9, 2022

pppedrillo commented Jan 9, 2022 • edited Loading

WireTownMan commented Jan 9, 2022

BernhardSchlegel commented Jan 9, 2022 • edited Loading

pppedrillo commented Jan 9, 2022 • edited Loading

thegreatgunbantoad commented Jan 10, 2022

thegreatgunbantoad left a comment

Choose a reason for hiding this comment

pppedrillo commented Jan 10, 2022 • edited Loading

pppedrillo left a comment

Choose a reason for hiding this comment

pppedrillo Jan 10, 2022

Choose a reason for hiding this comment

BernhardSchlegel Jan 10, 2022

Choose a reason for hiding this comment

pppedrillo left a comment • edited Loading

Choose a reason for hiding this comment

pppedrillo left a comment • edited Loading

Choose a reason for hiding this comment

thegreatgunbantoad commented Jan 10, 2022

pppedrillo commented Jan 10, 2022 • edited Loading

thegreatgunbantoad commented Jan 10, 2022 • edited Loading

thegreatgunbantoad commented Jan 10, 2022

thegreatgunbantoad commented Jan 10, 2022

BernhardSchlegel commented Feb 4, 2022

universam1 left a comment

Choose a reason for hiding this comment

BernhardSchlegel commented Feb 4, 2022

pppedrillo commented Jan 9, 2022 •

edited

Loading

BernhardSchlegel commented Jan 9, 2022 •

edited

Loading

pppedrillo commented Jan 9, 2022 •

edited

Loading

pppedrillo commented Jan 10, 2022 •

edited

Loading

pppedrillo left a comment •

edited

Loading

pppedrillo left a comment •

edited

Loading

pppedrillo commented Jan 10, 2022 •

edited

Loading

thegreatgunbantoad commented Jan 10, 2022 •

edited

Loading