Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added null api for not pushing readings to any consumer. #70

Merged
merged 1 commit into from
Dec 23, 2014

Conversation

r00t-
Copy link
Contributor

@r00t- r00t- commented Dec 23, 2014

i like this.
but we should leave the volkszaehler api as default (with a warning maybe),
and accept the old "vz" value,
to not break people's configurations.

@andig
Copy link
Contributor

andig commented Dec 23, 2014

but we should leave the volkszaehler api as default (with a warning maybe), and accept the old "vz" value.

Fixed. Any unknown value defaults to the vz api. Console message now clearly mentions this.

@r00t-
Copy link
Contributor Author

r00t- commented Dec 23, 2014

pointless details, fix them if you care, else i'd just merge it:
@file Null.hpp * Header file for volkszaehler.org API calls
@file Null.cpp * Header file for null API calls
the config has one channel with api "volkszaehler" and the others with "vz"
maybe name it NullApi instead of just "Null" to make it less confusing
(even if theoretically the namespace and directory are enough)
whitespace changes in Volkszaehler.hpp

@r00t-
Copy link
Contributor Author

r00t- commented Dec 23, 2014

oh, and no need to warn if the api "vz" (or "volkszaehler") is explicitly set ;)

@andig
Copy link
Contributor

andig commented Dec 23, 2014

Breaking change: renamed channel->protocol to channel->api for sake of distinction. Should not hurt unless you're a mysmartgrid user.

@andig
Copy link
Contributor

andig commented Dec 23, 2014

@file Null.hpp * Header file for volkszaehler.org API calls

Fixed. Would not change the Null file name due to api namespace as you've mentioned.

@andig
Copy link
Contributor

andig commented Dec 23, 2014

TODO: add documentation section following http://wiki.volkszaehler.org/software/controller/vzlogger#meters_protocols once commit is done

@r00t-
Copy link
Contributor Author

r00t- commented Dec 23, 2014

if you're getting tired of it, just merge it...

but i'm still unhappy with how the defaulting is handled...
an explicitly set value should not be ignored when it's unknown.

Meter_Options does:
if (apiProtocol_str == NULL) { apiProtocol_str = strdup("volkszaehler");
(which i missed because it was not even part of your changes.
i was assuming you had changed the name from"vz" to 'volkszaehler"
due to your changes in the config)
and then MeterMap does not actually handle that value explicitly.
if the defaulting when the api is not explicltly specified is done in the config parser already,
metermap could (and should) throw an error on unknown values.

maybe i'll just make a new fork with my changes ;)

@r00t-
Copy link
Contributor Author

r00t- commented Dec 23, 2014

i would probably remove the handling of the NULL in Meter_Options,
pass the NULL to MeterMap,
and handle it there.

andig added a commit that referenced this pull request Dec 23, 2014
Added null api for not pushing readings to any consumer.
@andig andig merged commit d7090bd into volkszaehler:master Dec 23, 2014
@andig
Copy link
Contributor

andig commented Dec 23, 2014

but i'm still unhappy with how the defaulting is handled...

Understood. I'd be happy if you could fork and take it from there- enough fiddling for one day...

Merry Christmas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants