Skip to content

Commit

Permalink
P1 Smart Meter reduce update interval (#1191)
Browse files Browse the repository at this point in the history 
* P1 Smart Meter reduce update interval

* P1 Smart meter reduced interval to min 10 sec
  • Loading branch information
@jvandenbroek @gizmocuz
jvandenbroek authored and gizmocuz committed Feb 2, 2017
1 parent 69cd1ed commit 2f1cb58
Showing 1 changed file with 16 additions and 17 deletions.
33 changes: 16 additions & 17 deletions hardware/P1MeterBase.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@
#define CRC16_ARC 0x8005
#define CRC16_ARC_REFL 0xA001

typedef enum {
ID=0,
STD,
LINE17,
LINE18,
EXCLMARK
typedef enum {
ID=0,
STD,
LINE17,
LINE18,
EXCLMARK
} MatchType;

#define P1_SMID "/" // Smart Meter ID. Used to detect start of telegram.
Expand Down Expand Up @@ -131,22 +131,22 @@ bool P1MeterBase::MatchLine()
m_linecount=1;
found=1;
}
else
else
continue;
break;
case STD:
if(strncmp(t.key, (const char*)&l_buffer, strlen(t.key)) == 0) {
found=1;
}
else
else
continue;
break;
case LINE17:
if(strncmp(t.key, (const char*)&l_buffer, strlen(t.key)) == 0) {
m_linecount = 17;
found=1;
}
else
else
continue;
break;
case LINE18:
Expand All @@ -159,26 +159,25 @@ bool P1MeterBase::MatchLine()
l_exclmarkfound=1;
found=1;
}
else
else
continue;
break;
default:
continue;
} //switch

if(!found)
continue;

if (l_exclmarkfound) {
time_t atime=mytime(NULL);
sDecodeRXMessage(this, (const unsigned char *)&m_p1power, "Power", 255);
bool bSend2Shared=(difftime(atime,m_lastSharedSendElectra)>59);
if (std::abs(double(m_lastelectrausage)-double(m_p1power.usagecurrent))>40)
bSend2Shared=true;
if (bSend2Shared)
if (
(std::abs(double(m_lastelectrausage)-double(m_p1power.usagecurrent))>40)&&
(difftime(atime,m_lastSharedSendElectra)>9))
{
m_lastelectrausage=m_p1power.usagecurrent;
m_lastSharedSendElectra=atime;
sDecodeRXMessage(this, (const unsigned char *)&m_p1power, "Power", 255);
}
if (
(m_p1gas.gasusage!=m_lastgasusage)||
Expand Down Expand Up @@ -288,7 +287,7 @@ bool P1MeterBase::MatchLine()


/*
/ GB3: DSMR 4.0 defines a CRC checksum at the end of the message, calculated from
/ GB3: DSMR 4.0 defines a CRC checksum at the end of the message, calculated from
/ and including the message starting character '/' upto and including the message
/ end character '!'. According to the specs the CRC is a 16bit checksum using the
/ polynomial x^16 + x^15 + x^2 + 1, however input/output are reflected.
Expand Down

14 comments on commit 2f1cb58

@jvandenbroek
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just compiled it with this code and now I actually see what the usagecurrent stands for.. I thought it was the difference in cumulative usage between two measurements, not the actual current watt usage! That's why I thought it would always be true, unless less than 40 watts were used (which to my knowledge is almost impossible ;-)) Stupid me, it seems that just moving the line sDecodeRXMessage under the if bSend2Shared statement should be enough to fix the CPU load issue I had.

Should I create a new pull request with the following in place, or will you just replace it?:

		((std::abs(double(m_lastelectrausage)-double(m_p1power.usagecurrent))>40)&&
				(difftime(atime,m_lastSharedSendElectra)>9))||
				(difftime(atime,m_lastSharedSendElectra)>59)

@gizmocuz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please, that sounds perfect

@gordonb3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be wrong. See: https://www.domoticz.com/forum/viewtopic.php?f=14&t=15847#p118402

I'm not real sure about the original intention of the bSend2Shared boolean. Looks like the original code did a conditional update of Power only, just like Gas, but it would appear that the sDecodeRXMessage for Power was taken outside the if statement a long time ago when Domoticz was still on subversion.

We should probably lose the reference to m_lastelectrausage as this appears to have had no function prior to this commit:

	time_t atime=mytime(NULL);
	if (difftime(atime,m_lastSharedSendElectra)>9))
	{
		sDecodeRXMessage(this, (const unsigned char *)&m_p1power, "Power", 255);
		m_lastSharedSendElectra=atime;
	}
	if (
		(m_p1gas.gasusage!=m_lastgasusage)||

@jvandenbroek
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's correct that it was placed outside the if statement, which seemed to be unintended. However, it might be useful to decrease the wattage difference of 40 to something less (maybe 20 or 10 watts?), so that it will update more often on smaller changes. However I think it's unnecessary to always update it every 10 seconds, this seems to generate a substantial amount of load on slower devices like the Pi 1 and I think we should always try to keep the load as low as possible anyway.

I don't have solar panels, but isn't the power usage counter always a positive value? I suppose that usage and delivery values are separate from each other and not being calculated.

@adgoudsmit
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are indeed separate values. But i still would like to have the 10 seconds update from my smartmeter.
Next to that, it might still be possible there is a moment in time the usagecurrent, and returncurrent are both 0 due to consuming the same as the solar panels deliver. Even in that situation i like the smart meter data to be updated.

Maybe make it configurable? For my smartmeter no limitation is needed.

@gordonb3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do get your point (I run Domoticz on an ARMv5 machine which has even less processing power than a Pi 1) and I did agree on this change at first but it does appear we overlooked something here.

Thing being that if you generate enough power behind the meter than usage will be zero. There is also no check for delivery which you might regard as negative usage but is recorded in a different counter. And of course the current usage (or delivery) doesn't say anything about the amount that was used since the last update, so we should in fact be looking at the sum of powerusage1 and powerusage2 rather than usagecurrent anyway.

@gordonb3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still thinking or disagreeing?

@gizmocuz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could try this

IF
current_usage - last_usage > xx

OR
current_delivery - last_delivery > xx

then send new sensor value

xx needs to be discussed...

OR

why not simply:

if (current_time - last_send_time) >=10 then
send

so we get an update every 10 seconds...

(or maybe 9)

I dont see the problem

This whole discussion was started because there was 1 user... that has a new meter, that updates the power/gas every second (very nice!)
This caused his cpu to go to 1.2 % ... yeahhhhh.... mayor... high... critical (am i sarcastic ? hope not)

Normally the meter sends an update every 10 seconds (old meters) for power, and once an hour for gas
So, to not miss a reading because of a clock tick, maybe if (>= 9 seconds) do update
This will probably cause an update every 9 seconds for 'this' user, but i think the CPU should be able to handle such a high load.

@gordonb3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I did see that conversation. Apparently the DSMR 5.x generation meters push datagrams every second and I do agree that there should be a rate limit for this. Do question what could be the bottle neck here by the way. This patch only stops the data from being written to the database, it will still process and do all the validation checks and logging of errors for every received datagram.

And I also question this: current_usage is the power that is drawn at that very moment. That is Watts, not Watt hours. Are current usage and current delivery, being snapshots of fractions of seconds, really the numbers we are most interested in or is it the counter values?

@jvandenbroek
Copy link
Contributor Author

@jvandenbroek jvandenbroek commented on 2f1cb58 Feb 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a pull request which would satisfy all users: #1212

Btw @gizmocuz, as already stated, I expressed it in system load values, which actually being 70-80% more CPU usage all the time ;-)

@jvandenbroek
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gordonb3 You're correct that all serial data is still being processed, however this really doesn't seem to generate much load with my DSMR 5 meter. Patching it so that serial (or LAN) only processes on some interval is more difficult, so I decided to just rate limit the updates.

@gizmocuz
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvandenbroek, yes i know what you stated , but more CPU does not reflect a 60% real cpu.
Thanks for your pull request, i do have a request there

@gordonb3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so that would mean the bottle neck is probably in the file I/O related to data storage. I guess a Pi might have trouble with that, using a flash card for storage and running all its peripherals over USB.

@jvandenbroek
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, I'm running the DB on a NFS share, which make things not better ;-) So actually it's indeed more I/O related than CPU.

Please sign in to comment.