WIP: Support Modbus Meters #272
Conversation
|
Wow, great addition! |
src/protocols/MeterModbus.cpp
Outdated
|
|
||
| ssize_t MeterModbus::read(std::vector<Reading> &rds, size_t n) { | ||
| try { | ||
| rds = _regmap->read(_mbconn); |
There was a problem hiding this comment.
Can you check what happens if read(_mbconn) returns 3 values but n is smaller? I didn't check what the assign operator does in this case.
There was a problem hiding this comment.
That's not clean at the moment! Currently all the read functions (except this one) overwrite pre-constructed Reading objects in a vector. But this moves a new vector over the old one.
There was a problem hiding this comment.
The background is, that I wondered why we pre-create empty Readings in the reading_thread and replace them in the Protocol::read. I assume it's left from the original C codebase. I think this makes no sense, so I am elaborating more C++ish read functions. Those return a vector with all their readings. The size is flexible, and everything would be less verbose. It would look like
std::vector<Reading> Meter::read();The Readings are returned by value and moved (C++11 move) on return.
Just like the RegisterMap::read() method. However this is just an idea, and thus MeterModbus::read adapts it to the exisiting scheme in a hacky way ;)
A Reading can be created like
rds.push_back(Reading(value, new StringIdentifier("TotalExpWh")))instead of
if (ret < n) {
rds[ret].identifier(new ObisIdentifier("2.7.0"));
rds[ret].value(get_record_value(record));
rds[ret].time();
++ret;
}There was a problem hiding this comment.
I fully agree. This is the preferred solution. We should even get rid of this parameter "n" and change all read functions to properly add (push_back) their values.
|
Good work! Let me know if you need help with the mocks. |
|
ping @flyingflo would be nice to get this in, @mbehr has already offered his help. Any chance to complete this PR? I would love to see modbus support as this would allow moving my entire monitoring infrastructure to standard hardware without the need for S0. |
|
Hi, chances are high. I haven't touched the code since a while, but its running since then. I have to sort out my branches to update the PR to the latest version. Still, no mocks yet.. |
This is useful to create a sensible reading with current time in one line.
This class is just a collection of functions, no data members. Make the member functions static and save us the creation of a useless object.
The what message must not be a automatic variable of the what() function, of course.
On Modbus RTU connections and on Modbus TCP connecitons multiple devices can be connected to one "connection". In vzlogger, we must implement this in one Meter, because access to the devices must be synchronized.
Depending on the meter connection, reading can take a significant part of the interval time. Record the time spent with reading, and wait only the time left until starting the next reading.
flyingflo
force-pushed
the
upstr-modbus2
branch
from
2b83a1b
to
5649f91
Compare
|
Updated with all patches on that topic that I currently have. Todo
|
This reverts commit 0492b2c.
Follow the existing seperation of Meters and Readings.
|
Todo
|
| } | ||
| }, | ||
| { | ||
| "enabled": false, |
| @@ -53,6 +53,7 @@ if( MBUS_FOUND ) | |||
| target_link_libraries(vzlogger ${MBUS_LIBRARY}) | |||
| endif( MBUS_FOUND ) | |||
| target_link_libraries(vzlogger ${OCR_LIBRARIES}) | |||
| target_link_libraries(vzlogger modbus) | |||
There was a problem hiding this comment.
Different from mbus_library?
There was a problem hiding this comment.
Yes, modbus and mbus have little in common, except the m and the bus ;)
| @@ -72,6 +73,7 @@ static const meter_details_t protocols[] = { | |||
| #ifdef OMS_SUPPORT | |||
| METER_DETAIL(oms, OMS, "OMS (M-BUS) protocol based devices", 100, false), // todo what is the max. amount of reading according to spec? | |||
| #endif | |||
| METER_DETAIL(modbus, Modbus, "Modbus meter", 64, true), | |||
There was a problem hiding this comment.
Surround with ifdef fpor modbus support throughout the vode?
There was a problem hiding this comment.
It belongs to this todo to make the whole modbus stuff optional
- Add CMake module for libmodbus and make this feature optional
Still open ..
I got a failure according to the tests. If I redo 9895172 the build gets succesfully completed. |
| std::map<std::string, RegisterMap::Ptr (*)()> RegisterMap::maps = { | ||
| { "ohmpilot", createMap<OpRegisterMap> }, | ||
| { "imemeter", createMap<IMEmeterRegisterMap> } | ||
| }; |
There was a problem hiding this comment.
Oh. Oooooh...
Nice to have an "easy way" to adresse known type of meters, but:
This is inflexible! For every new meter or single parameter we need somebody to understand and change the code and users have to compile again.
There was a problem hiding this comment.
I totally agree. Its inflexible. There would be a standard from Sunspec to describe Modbus registers in an xml format. However, this would pull in an xml parser requires much more work to be done correctly. Anyways, it would be a good idea to do.
There was a problem hiding this comment.
If we implement a standard register map, that would add support all compliant meters.
My two example devices don't follow any standard maps, they are totally custom.
| void OpRegisterMap::read(std::vector<Reading>& rds, ModbusConnection::Ptr conn, unsigned id) { | ||
| uint16_t regs[10]; | ||
|
|
||
| conn->read_registers(40799, 10, regs, id); |
| uint16_t regs[reg_len]; | ||
| int value; | ||
|
|
||
| conn->read_registers(reg_offset, reg_len, regs, id); |
There was a problem hiding this comment.
...variables.
Pick one way and stick with it, please.
There was a problem hiding this comment.
Easy fix, these constants are only used once..
| conn->read_registers(40799, 10, regs, id); | ||
| rds.push_back(Reading(regs[9] / 10.0, new ModbusReadingIdentifier(id, "T"))); | ||
| rds.push_back(Reading(regs[2], new ModbusReadingIdentifier(id, "P"))); | ||
| rds.push_back(Reading(regs[8], new ModbusReadingIdentifier(id, "E"))); |
There was a problem hiding this comment.
Whats does T, P and E mean?
There was a problem hiding this comment.
All this register maps only make sense if you know the device. In this case its Power, Temperature, Energy.
I think this is caused by minihttpd missing and the test code assuming that its there without any checks. |
Ich bin seit einiger Zeit auch bei gosdm engagiert- da gibt es das gleiche Thema: gonium/gosdm630#41. Für GoSDM gibt es folgende Anforderungen an eine "meter definition":
@flyingflo wäre das auch für Deine Zähler machbar?
Bäh, XML :/ Wenn wir es geschickt machen könnten wir beide Projekte mit Definitionsdateien unterstützen, der Einfachheit JSON (kein zusätzlicher großer Parser) oder auch YAML. GoSDM kann mittlerweile auch MQTT. Eine generelle Frage wäre ob wir immer noch mehr Geräte in den vzlogger einbauen müssen oder uns vielleicht noch eine MQTT -> Middleware (ggf. plus WebSocket/ Httpd/ Push) Bridge einfallen lassen wollen. Alternativ vzlogger einen MQTT Client verpassen um die Aggregationsfunktionen zu nutzen. Das wäre eine schöne Diskussion für das Entwicklertreffen. |
|
@flyingflo wenn Du magst können wir diese Zähler auch bei GoSDM hinzufügen. |
Before test of this PR i made sure the actual head:master is compiling and working fine. Libmicrohttpd-dev is installed. |
My first thought was a secondary config, too. But i don't want to push a solution because i will not be able to implement it.
I noticed when i searched for doku of the ohmpilot registers. |
Interessant. Muss ich mich mal einlesen... |
Please rebase on master. I think this has been only recently fixed. |
|
Wenn es durch das Register-Mapping kompliziert wird: könnte man das nicht erstmal weglassen? Wäre es nicht prinzipiell ausreichend (und maximal flexibel), die gewünschten Register und den Datentyp in der vzlogger.conf im Channel-Kontext anzugeben? +1 für MQTT-Support, wäre imho sowohl für vzlogger als auch für die MW eine sehr sinnvolle Erweiterung. |
|
On Freitag, 18. Mai 2018 08:14:37 CEST andig wrote:
Für GoSDM gibt es folgende Anforderungen an eine "meter definition":
Mapping Messwert (im Prinzip OBIS KZ) -> Modbus Register
Definition Ausleseregister für den Zählertyp
Definition eines Registers dass nur dieser Zähler unterstützt um eine
automatische Erkennung zu ermöglichen @flyingflo wäre das auch für Deine
Zähler machbar?
Ja, ich denke schon. Wichtig ist noch, dass man einen sinnvollen Registerblock
als ganzes in einem Modbus Zugriff ausliest, und nicht jeden Eintrag im
Mapping extra, das geht sehr langsam.
|
Ups, da hast Du mich erwischt. Gosdm macht das einzeln. Es sollte allerdings nicht schwer sein aus einzelnen Registerdefinitionen sinnvolle Blöcke rauszusuchen die dann gelesen werden. Im Prinzip soviele Register bis eine Lücke auftaucht. Ich schau mal ob sich das für Gosdm sinnvoll basteln lässt. |
|
Hallo, nachdem der PR immer noch herumliegt.. |
|
@flyingflo erstmal vielen Dank für die Arbeit die Du in diesen PR gesteckt hast! Ich bin dennoch kein Fan davon da wir damit vzlogger immer massiver machen. Gleichzeitig gibt es andere Projekte die in der Lage wären Datenerfassung und Volkszähler Anbindung zu modularisieren: Der Vorteil von GoSDM ist dass bereits eine Reihe von Modbus Zählern und Wechselrichtern implementiert sind. Ich sehe keinen Wert darin diesen Aufwand nochmal in eine Parallelentwicklung in vzlogger zu investieren? |
|
@flyingflo hast Du Dir mal gosdm angeschaut? |
This is not done yet. Just wanted to show you the idea.
Todo