search

spacemanspiff2007 / sml2mqtt

Sml to MQTT Bridge
GNU General Public License v3.0
25 stars 8 forks source link

Feedback on version 1.0 #7

Closed git-developer closed 2 years ago

git-developer commented 2 years ago

I've stumbled across a discussion on the changes towards version 1.0 in issue #6. I'd like to add a few points but don't want to pollute the existing issue with my comments, so I decided to open this separate issue.

1.) Bug: Indentation of inline comments is changed on program start

I noticed that every time sml2mqtt is started, the configuration is re-written and inline comments are moved 1 space to the right.

Negative side effect: the configuration is no longer allowed to be read-only. Up to version 0.6, it was possible to start the program with a read-only configuration file as long as it was complete and correct.

2.) Suggestion: Add support for a simpler configuration syntax

Version 1.0 offers support for value-dependent configuration. A great feature, thanks for that. It has a downside though: the configuration file is getting quite bloated because of mqttand topic properties (already reported here). Example:

devices:
  0123456789abcde01234:
    mqtt:
      topic: power_meter
    status:
      topic: status
    skip:
    - 0100010801ff
    - 0100020801ff
    values:
      0100010800ff:                     # 1.8.0   Positive active energy (A+) total [kWh]
        mqtt:
          topic: total_energy_import
      0100010801ff:                     # 1.8.1   Positive active energy (A+) in tariff T1 [kWh]
        mqtt:
          topic: t1_energy_import
      0100020800ff:                     # 2.8.0   Negative active energy (A+) total [kWh]
        mqtt:
          topic: total_energy_export
      0100020801ff:                     # 2.8.1   Negative active energy (A+) in tariff T1 [kWh]
        mqtt:
          topic: t1_energy_export
      0100100700ff:                     # 16.7.0  Sum active instantaneous power (A+ - A-) [kW]
        mqtt:
          topic: power

My suggestion is to add support for a "shortcut" for mqtt topics. Technically speaking, this means to allow a string value for the mqtt property and parents of an mqtt property when no other child properties are required. This is meant to be in addition to the current syntax, not as a replacement. Example:

  0123456789abcde01234:
    mqtt: power_meter
    status: status
    skip:
    - 0100010801ff
    - 0100020801ff
    values:
      0100010800ff: total_energy_import # 1.8.0   Positive active energy (A+) total [kWh]
      0100010801ff: t1_energy_import    # 1.8.1   Positive active energy (A+) in tariff T1 [kWh]
      0100020800ff: total_energy_export # 2.8.0   Negative active energy (A+) total [kWh]
      0100020801ff: t1_energy_export    # 2.8.1   Negative active energy (A+) in tariff T1 [kWh]
      0100100700ff: power               # 16.7.0  Sum active instantaneous power (A+ - A-) [kW]

3.) Suggestion: Add support for an mqtt topic in skip list

This feature is not really important, but would make the configuration easier to read: The skip property should allow an mqtt topic value additionally to an OBIS value.

Example for current syntax:

devices:
  0123456789abcde01234:
    skip:
    - 0100010801ff
    - 0100020801ff

Example for suggested syntax:

  0123456789abcde01234:
    skip:
    - t1_energy_import
    - t1_energy_export
    values:
      0100010801ff: t1_energy_import
      0100020801ff: t1_energy_export

4.) Suggestion: Label release versions with a Git Tag

Some commits comments start with a version number (0.6 and 1.0). I'd prefer a git tag for each release.

The most important thing comes last: thanks for this great project! It's really helpful and easy to use. Keep up the good work!

spacemanspiff2007 commented 2 years ago

Thank you for your feedback! It's really helpful.

1.) Bug: Indentation of inline comments is changed on program start

I'm currently not happy with the configuration library so this will change and it's a bad bug!

2.) Suggestion: Add support for a simpler configuration syntax

How about omitting the topic sub entry? That's something I could easily implement. The only downside: It's not so clear any more that it's possible to override qos retain or to set a full topic that is not a fragment with full_topic

      0100020801ff:                     # 2.8.1   Negative active energy (A+) in tariff T1 [kWh]
        mqtt: t1_energy_export

What do you think?

3.) Suggestion: Add support for an mqtt topic in skip list

Maybe I can move the skip property in the config:

      0100020801ff:                     # 2.8.1   Negative active energy (A+) in tariff T1 [kWh]
        mqtt: t1_energy_export
        skip: True

However it makes no sense to set a topic and skip the value, so it's currently unclear to me what the goal is.

4.) Suggestion: Label release versions with a Git Tag

They are - both are testing versions to get feedback. The last official release was 0.5 which has a tag set accordingly.

git-developer commented 2 years ago

Thanks for your instant feedback!

2.) Suggestion: Add support for a simpler configuration syntax

How about omitting the topic sub entry?

If I understand you correctly, both of the following formats would be allowed then:

I think that would be a nice improvement! When both options are documented properly, I see no problems concerning qos, retain and full_topic. So +1 for omitting the topic sub entry.

Maybe it's possible to do the same for the parent properties of mqtt so that a third option would be

I don't know about the difficulty on implementing this, though.

Side note: what do you think of using the term default instead of base for the default values in the configuration file?

3.) Suggestion: Add support for an mqtt topic in skip list

it's currently unclear to me what the goal is

The benefit is that you can see in clear text what you skip, instead of a cryptic OBIS value only. But I admit it's confusing to configure a skipped property. After thinking about it, i guess it should be enough to use a simple comment like this:

devices:
  0123456789abcde01234:
    skip:
    - 0100010801ff # t1_energy_import / 1.8.1   Positive active energy (A+) in tariff T1 [kWh]
    - 0100020801ff # t1_energy_export / 2.8.1   Negative active energy (A+) in tariff T1 [kWh]

Thanks for pointing this out, I take back suggestion 3.

spacemanspiff2007 commented 2 years ago

If I understand you correctly, both of the following formats would be allowed then:

correct!

Maybe it's possible to do the same for the parent properties of mqtt so that a third option would be

That won't work - sorry

Side note: what do you think of using the term default instead of base for the default values in the configuration file?

I've had default first because for qos and retain it's the correct wording. However the topics are fragments (except if you configure full_topic in a child config entry) So these configuration entries build the full topic according to all fragments so using default for the topmost topic is kind of misleading. That's why I went with base. Do you think default still makes sense?

git-developer commented 2 years ago

Feels like the base topic and the default values are different things. What about separating them:

mqtt:
  ...
  topic prefix: sml2mqtt
  defaults:
    qos: 0
    retain: 0
  last will: status

topic prefix could be base topic or common topic, too.

spacemanspiff2007 commented 2 years ago

I fixed the indentation bug in the latest version and split the topics. Would you mind giving it a try?

Note: You have to update easyconfig to 0.2.1 for it to work.

git-developer commented 2 years ago

It seems the topic split is broken, the program doesn't startup anymore. #9 should fix it. Is it possible to avoid such a problem by a test?

The bug (1.) is fixed: comments are not shifted anymore, and the config file may be used read-only. Thanks!

spacemanspiff2007 commented 2 years ago

I've made a release 1.0. If you have further isses/hints/feedback just post it here.

git-developer commented 2 years ago

Thanks! Here is my feedback:

  1. The application is still broken. It does not start up because base is still in use. You closed my PR without merging the patch that resolves this problem.
  2. The default MQTT port in the configuration should be 1883 instead of 1833 (here and here).
  3. The git history looks strange. The last 2 commits have no committer and the date is wrong. I mention this because it breaks the update check of my automatic Docker builds:
$ git log
commit b0877209a2d5878f3eec7ee59692980199105b6c (HEAD -> master, tag: 1.0, origin/master, origin/HEAD)
Author: - <->
Date:   Fri Oct 15 13:33:30 2021 +0200

    1.0:
    - new configuration format with many new features

commit c981d1b4effc930fbe387d9ac82bf0a8167cc868
Author: - <->
Date:   Wed Oct 13 07:38:34 2021 +0200

    0.6:
    - Dump frame on error
    - restart service on failure (closes #3)

commit 7064ff4c28394c0409f1a961dcab7e4c4362adde (tag: 0.5)
Author: spacemanspiff2007 <10754716+spacemanspiff2007@users.noreply.github.com>
Date:   Tue Aug 24 09:37:02 2021 +0200

    0.5: Bumped smllib dependency
spacemanspiff2007 commented 2 years ago

Seems like the daylight savings time jetlag is bigger than expected. I somehow missed the other changes in 1) but created a new version 1.0.1. Thanks for the feedback.

The git history looks like that because I merged multiple commits that I had together before pushing. I'll pay attention next time that date and author is properly set.

git-developer commented 2 years ago

Version 1.0.1 fixes all issues from above and works as expected. Thanks!