Add P155_Smartmeter#5542
Conversation
| :header: "Store", "Link" | ||
| :widths: 5, 40 | ||
|
|
||
| "eBay","`TTL/UART IR Reading Head <https://www.ebay.de/itm/314015465828>`_" |
There was a problem hiding this comment.
Maybe you could take a picture of the device you have (don't use pictures from the ebay page), as the ebay link may not remain to work forever.
| case PLUGIN_DEVICE_ADD: | ||
| { | ||
| Device[++deviceCount].Number = PLUGIN_ID_155; | ||
| Device[deviceCount].Type = DEVICE_TYPE_DUMMY; |
There was a problem hiding this comment.
dev.Type = DEVICE_TYPE_SERIAL;Also please use auto& dev = Device[++deviceCount]; and then replace Device[deviceCount] with dev
| The plugin uses Hardware Serial2 on fixed pins: | ||
|
|
||
| * RX = GPIO 16 | ||
| * TX = GPIO 17 |
There was a problem hiding this comment.
Ehh this should be using a configuration to select a port.
Please search the code for ESPEasySerial, as used in plugins like P052/P053/P054 etc.
Not all ESP boards have the same pins accessible.
For example on ESP32-classic exactly these 2 pins have issues as they can also be used for PSRAM. Even on a board without PSRAM these pins will show high frequency signals every now and then.
I noticed you are using ESPEasySerial to create the UART instance, but still using hard coded values.
When using the dev.type (in the PLUGIN_ADD) to be serial, you already have the selector present on the webpage.
Then you could add something like this:
case PLUGIN_WEBFORM_SHOW_SERIAL_PARAMS:
{
String options_baudrate[6];
for (int i = 0; i < 6; ++i) {
options_baudrate[i] = String(p085_storageValueToBaudrate(i));
}
const FormSelectorOptions selector(6, options_baudrate);
selector.addFormSelector(F("Baud Rate"), P085_BAUDRATE_LABEL, P085_BAUDRATE);
addUnit(F("baud"));
addFormNumericBox(F("Modbus Address"), P085_DEV_ID_LABEL, P085_DEV_ID, 1, 247);
break;
}to fine-tune the serial config (taken as example from P085)
|
|
||
| CONFIG_PORT = 5; // Serial2 | ||
| CONFIG_PIN1 = 16; | ||
| CONFIG_PIN2 = 17; |
There was a problem hiding this comment.
These are macros, you should not assign fixed values here.
const int16_t serial_rx = CONFIG_PIN1;
const int16_t serial_tx = CONFIG_PIN2;
const ESPEasySerialPort port = static_cast<ESPEasySerialPort>(CONFIG_PORT);| "11", "L2_A", "1-0:51.7.0", "A", "Current L2" | ||
| "12", "L3_A", "1-0:71.7.0", "A", "Current L3" | ||
|
|
||
| \* SML-Auto delivers energy values in Wh. Enter ``VALUE/1000`` in the formula field to get kWh. |
There was a problem hiding this comment.
I think the formula is %value%/1000
| .. note:: | ||
|
|
||
| With **SML-Auto**, energy registers deliver the raw value in Wh (scaler is read | ||
| automatically from the telegram). Enter ``VALUE/1000`` in the ESPEasy formula |
| // | ||
| // SML-Auto Funktionsweise: | ||
| // Nach der OBIS-Kennung enthält jeder SML-ListEntry folgende Felder, | ||
| // jeweils eingeleitet durch ein TL-Byte (Type-Length): |
There was a problem hiding this comment.
Please use English, also in comments
|
|
||
| #include <ESPeasySerial.h> | ||
|
|
||
| ESPeasySerial *P155_MySerial = nullptr; |
There was a problem hiding this comment.
Usually we do have a PluginStruct for plugin runtime data so we can have multiple instances of a plugin.
Also this makes sure no unused global variables are allocated in memory. (like nearly everything below this line upto line 206)
But we can change this later.
| { | ||
| String p155_rxID = "x-x:x.x.x*x"; | ||
| float value; | ||
| p155_dataStructD0(String xID, float xvalue) |
There was a problem hiding this comment.
By calling this function, you essentially copy the string.
Better to use p155_dataStructD0(const String& xID, float xvalue) as declaration of the constructor.
And since you don't actually do anything with the arguments, it is even better to have the constructor like this:
p155_dataStructD0(const String& xID, float xvalue) : p155_rxID(xID), value(xvalue) {}| break; | ||
| } | ||
| } | ||
| addLogMove(LOG_LEVEL_DEBUG, log); |
There was a problem hiding this comment.
If you declare the int i outside the scope of the for loop, you will still know which i was handled.
So then you can have all the log related lines together and wrap them into #ifndef BUILD_NO_DEBUG and if (loglevelActiveFor(LOG_LEVEL_DEBUG))
| { | ||
| String log = F("D0 Parse: ID="); | ||
| log += p155_rxID; | ||
| for (int i = 1; i < p155_outputOptionsAct; i++) |
There was a problem hiding this comment.
Why starting at 1 here? If you don't need that one, why have it declared in the p155_myDataD0 array?
| raw |= (int32_t)0xFFFFFF00; | ||
| else if (readBytes == 2 && (rawU & 0x8000)) | ||
| raw |= (int32_t)0xFFFF0000; | ||
| lvalue = (float)raw; |
There was a problem hiding this comment.
When assigning an int to a float type, you don't need these C-style casts
| } | ||
| else // unsigned (typ 6) oder unbekannt | ||
| { | ||
| lvalue = (float)rawU; |
|
|
||
| float scaledValue = (p155_scaler != 0) | ||
| ? lvalue * powf(10.0f, (float)p155_scaler) | ||
| : lvalue; |
There was a problem hiding this comment.
This scalar only seems to be set once in the code.
So why not calculate the scalar value there and have the scalar be either 1.0f or powf(10.0f, X) with X being the scalar value.
| float p155_readVal(uint8_t query, unsigned int model) | ||
| { | ||
| if (model == 0) | ||
| return p155_myDataD0[query].value; |
There was a problem hiding this comment.
There is no check for the range of the arrays
| uint8_t p155_charsRead = 0; | ||
| char p155_rxBuffer[P155_RX_BUFFER]; | ||
| char p155_ringBuffer[8]; | ||
| byte p155_rxOrbis[6]; //|1|0|2|8|0|255 |
There was a problem hiding this comment.
Arrays are not initialized.
char p155_rxBuffer[P155_RX_BUFFER]{};
char p155_ringBuffer[8]{};
byte p155_rxOrbis[6]{}; //|1|0|2|8|0|255Adding {} will initialize them with all zeroes.
| int p155_anzBytes; | ||
| int p155_posDataAct; | ||
| int p155_registerAct; | ||
| int p155_outputOptionsAct; |
There was a problem hiding this comment.
Int values also not initialized.
int p155_anzBytes{};
int p155_posDataAct{};
int p155_registerAct{};
int p155_outputOptionsAct{};This will initialize them with zeroes.
| String logdata1 = F("D0: Data="); | ||
|
|
||
| unsigned long timeOut = millis() + 10; | ||
| while (P155_MySerial->available() && millis() < timeOut) |
There was a problem hiding this comment.
Please use functions like timePassedSince() so we can be sure those time calculations are done correct.
Otherwise you may get odd errors every 49.7 days :)
Also available() can be a rather expensive function, so maybe better to store it before making the while loop and if there is time left, you can do another check for available().
const unsigned long start = millis();
size_t available = P155_MySerial->available();
while (available && timePassedSince(start) < 10) {
--available;
if (available == 0) { available = P155_MySerial->available(); }
...|
|
||
| if (c == '(') | ||
| { | ||
| p155_rxBuffer[p155_charsRead] = '\0'; |
There was a problem hiding this comment.
There is no check for p155_charsRead < NR_ELEMENTS(p155_rxBuffer)
Should be done at the start of the while loop. (or at the end as you may set this value to 0 in this loop)
No description provided.