-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add support for SDM230 ModBus #13443
Conversation
@Noschvie If you want to test, you can clone directly from the author's fork and compile yourself: https://github.com/dad401/Tasmota |
I'm a bit confused. What's the reason for closing this PR? It's not merged or do I miss something? |
It should not have been closed. No idea what the relation was with the other PR. |
No need to think about it. You already have too many registers present. I suggest to remove Max Power and Phase Angle. They are not within the Tasmota data set currently and shouldn't be either. The issue is that gathering the registers takes too much time and is flooding the gui. You might want to enable profiling to see how much time is spend in the driver gathering the registers. |
In my opinion these registers "must be" included for this device:
These ones are "should be":
And these ones are "nice to have":
I suggest to change the "must be" block as default and the "should be" block as optional (#define) including a warning about possible timing issues. I'm underway to get profiling to work and get those timings for the current source. |
I tried to enable profiling. I added Any hints? |
Pls retry using latest dev. To enable profiling use these defines:
|
rebased my fork with your current development branch (first time ever, not sure if something went wrong, but seems to be ok) and compiled it. Now it is working!
Here is the output I hope you expected (removed all DMP messages):
|
new log after redesign using #define SDM230_MORE_REGS (3 extra modbus registers)
|
If you don't own a SDM230 device you can use the enclosed python script (thanks to modbus-tk) It simulates a SDM230 slave with all registers used in this driver. For me it's working between a PI and Wemos D1 using USB-RS485 / TTL-RS485 adapters. I assume that it also works with no adapters. |
This PR has been automatically marked as stale because it hasn't any activity in last few weeks. It will be closed if no further activity occurs. Thank you for your contributions. |
This PR was automatically closed because of being stale. |
There are conflicts to be resolved before merging. |
fixed it - What else has to be done to make this PR final? |
Was there any holdup for this PR? |
tasmota/tasmota_configurations.h
Outdated
@@ -199,6 +199,7 @@ | |||
#define USE_MCP39F501 // Add support for MCP39F501 Energy monitor as used in Shelly 2 (+3k1 code) | |||
#define USE_SDM72 // Add support for Eastron SDM72-Modbus energy monitor (+0k3 code) | |||
#define USE_SDM120 // Add support for Eastron SDM120-Modbus energy monitor (+1k1 code) | |||
#define USE_SDM230 // Add support for Eastron SDM230-Modbus energy monitor (+?? code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncomment as there is no room for additional sensors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uncommented
CHANGELOG.md
Outdated
@@ -2,6 +2,9 @@ | |||
All notable changes to this project will be documented in this file. | |||
|
|||
## [Unreleased] - Development | |||
- added support (POC) for Eastron SDM230 ModBus (based on existing drivers SDM120 and SDM630) - suggestion for future versions: | |||
use the same pin names for all SDM(72|120|220|230|630) drivers, e.g.: GPIO_SDMxxx_TX and GPIO_SDMxxx_RX and integrate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove nonsense information from changelog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tasmota/tasmota_template.h
Outdated
@@ -176,6 +176,7 @@ enum UserSelectablePins { | |||
GPIO_BL0942_RX, // BL0942 Serial interface | |||
GPIO_HM330X_SET, // HM330X SET pin (sleep when low) | |||
GPIO_HEARTBEAT, GPIO_HEARTBEAT_INV, | |||
GPIO_SDM230_TX, GPIO_SDM230_RX, // SDM230 Serial interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be added at end of list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the end. By the way - what's the purpose for that?
tasmota/tasmota_template.h
Outdated
@@ -373,7 +374,7 @@ const char kSensorNames[] PROGMEM = | |||
D_SENSOR_BL0942_RX "|" | |||
D_SENSOR_HM330X_SET "|" | |||
D_SENSOR_HEARTBEAT "|" D_SENSOR_HEARTBEAT "_i|" | |||
|
|||
D_SENSOR_SDM230_TX "|" D_SENSOR_SDM230_RX "|" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tasmota/xdrv_03_energy.ino
Outdated
@@ -1108,7 +1108,7 @@ void EnergyShow(bool json) { | |||
EnergyFormatSum(value2_chr, Energy.daily, Settings->flag2.energy_resolution, json)); | |||
|
|||
/* | |||
#if defined(SDM630_IMPORT) || defined(SDM72_IMPEXP) | |||
#if defined(SDM630_IMPORT) || defined(SDM72_IMPEXP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't touch unchanged files globbering the PR. Remove spaces at end of line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed space
@dad401 : are you willing to complete this PR ? |
changed: CHANGELOG.md changed: tasmota/language/de_DE.h changed: tasmota/language/en_GB.h open/todo: update all other localization files changed: tasmota/my_user_config.h changed: tasmota/support_features.ino changed: tasmota/tasmota_configurations.h changed: tasmota/tasmota_configurations_ESP32.h changed: tasmota/tasmota_template.h changed: tasmota/tasmota_template_legacy.h changed: tasmota/xdrv_03_energy.ino new file: tasmota/xnrg_21_sdm230.ino added support (POC) for Eastron SDM230 ModBus (based on existing drivers SDM120 and SDM630) - suggestion for future versions: use the same pin names for all SDM(72|120|220|230|630) drivers, e.g.: GPIO_SDMxxx_TX and GPIO_SDMxxx_RX and integrate all drivers into one (xnrg_xx_SDM-ModBus.ino)
missed this change. Now tasmota-sensors compiles with success!
moved AGPIO(GPIO_SDM230_XX) to the end: tasmota/tasmota_template_legacy.h
…ith any addition."
- fixed comment for SDM230_SPEED define in xnrg_21_sdm230.ino
synced sdm230 driver default registers to generic energy driver values made maximum power, phase angle and resettable energy optional
Let me know if something else has to be changed... |
Add support for Eastron SDM230 modBus energy meter (#13443)
since this is a single phase counter, is it possible to run 3 counters in parallel over the rs485 for all phases to one tasmota unit and get it aggregated there? |
I don`t think so. Having more than one device, IMHO it`s better to go with the Smart Meter Interface. |
This is my first PR and use of GitHub - so do not grill me ;)
I added support (POC) for Eastron SDM230 ModBus (based on existing drivers SDM120 and SDM630).
As it is a lot of annoying work I did not changed all language files so far. In case this PR will be ignored.
I suggest for future versions:
I am not sure why there is an extra #define for the SDM630 Import Energy. Is it to save space? I also "copied" this feature for SDM230.
Description:
Related issue (if applicable): fixes #
Checklist:
NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass