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.

Sign up for GitHub

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

Jump to bottom

Implement Siemens PAC1600 meter #2396

Merged

sfeilmeier merged 18 commits into OpenEMS:develop from danzerslab:feature/meterSiemensPAC1600

Jan 2, 2024

Contributor

JosefRick commented Oct 23, 2023

Added Meter Siemens PAC1600

I tested it on a real Siemens PAC 1665 and found no issues until now.

JosefRick and others added 5 commits

October 17, 2023 13:29


          Update README.md

cb22501


          Implementing the meter Siemens PAC1600, tested with 7kt1665

855639d


          Update README.md

827ec17

delete the changes


          Revert "Update README.md"

5d91df7

This reverts commit 827ec17.


          Revert "Update README.md"

4c5de5c

This reverts commit cb22501.

Contributor

sfeilmeier commented Nov 3, 2023

@clehne Could anyone of your team do a first review of this?

tsicking requested changes

View reviewed changes

Contributor

tsicking left a comment

Thanks for the PR. My comments are mostly about general things. I haven't checked the modbus protocol in detail yet.
I would put all this into the bundle io.openems.edge.meter.siemens and create two packages there, one for the existing meters Siemens PAC 2200,... and one for your meter.
Also, in the tools directory, you have a script prepare-commit.sh. Running this script before you commit takes care of some errors regarding code style.

io.openems.edge.application/EdgeApp.bndrun Outdated Show resolved Hide resolved

io.openems.edge.application/EdgeApp.bndrun Outdated

rrd4j;version='[3.9.0,3.9.1)',\

Contributor

tsicking Nov 7, 2023

This reordering should be reverted to remain alphabetical ordering.

io.openems.edge.meter.siemens.pac1600/readme.adoc Outdated

+              = Siemens Meter PAC1600
+              Implemented Natures:
+              -ElectricityMeter

Contributor

tsicking Nov 7, 2023

Just to be consistent with other readme.adocs: There should be a whitespace after the "-".

io.openems.edge.meter.siemens.pac1600/.classpath Outdated

@@ @@ -0,0 +1,16 @@ @@
+              <?xml version="1.0" encoding="UTF-8"?>
+              <classpath>
+              	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-17">

Contributor

tsicking Nov 7, 2023

Comparing it with other .classpath files, I see that they don't have the <attributes> tag, and this classpath entry is second in the list.

io.openems.edge.meter.siemens.pac1600/.gitignore Outdated

		@@ -0,0 +1 @@
		/bin_test/

Contributor

tsicking Nov 7, 2023

This should also contain \bin\ and \generated\.

...meter.siemens.pac1600/src/io/openems/edge/meter/siemens/pac1600/MeterSiemensPAC1600Impl.java Outdated

+              		// this.config = config;
+              	}
+              	@Deactivate

Contributor

tsicking Nov 7, 2023

This should also have an @Override-Annotation.

...meter.siemens.pac1600/src/io/openems/edge/meter/siemens/pac1600/MeterSiemensPAC1600Impl.java Outdated

+              				"Modbus", config.modbus_id())) {
+              			return;
+              		}
+              		// this.config = config;

Contributor

tsicking Nov 7, 2023

This comment should be removed.

...meter.siemens.pac1600/src/io/openems/edge/meter/siemens/pac1600/MeterSiemensPAC1600Impl.java Outdated

+              	@Override
+              	public MeterType getMeterType() {
+              		return this.meterType;
+              		// return this.config.type();

Contributor

tsicking Nov 7, 2023

This comment should be removed.

...meter.siemens.pac1600/src/io/openems/edge/meter/siemens/pac1600/MeterSiemensPAC1600Impl.java Outdated

+              		immediate = true, //
+              		configurationPolicy = ConfigurationPolicy.REQUIRE //
+              )
+              public class MeterSiemensPAC1600Impl extends AbstractOpenemsModbusComponent

Contributor

tsicking Nov 7, 2023

Call this MeterSiemensPac1600Impl, as three upper case letters in a row are a checkstyle violation. Same for Interface and Test.

...meter.siemens.pac1600/src/io/openems/edge/meter/siemens/pac1600/MeterSiemensPAC1600Impl.java Outdated

+              					m(ElectricityMeter.ChannelId.ACTIVE_POWER, new SignedDoublewordElement(57),ElementToChannelConverter.SCALE_FACTOR_MINUS_2),
+              					m(ElectricityMeter.ChannelId.REACTIVE_POWER, new SignedDoublewordElement(59),ElementToChannelConverter.SCALE_FACTOR_MINUS_2)),
+              				new FC3ReadRegistersTask(6687, Priority.HIGH,

Contributor

tsicking Nov 7, 2023

The priority of this and the next task can be set to LOW, as it's not necessary to read energy values all that frequently.

JosefRick added 2 commits

November 9, 2023 14:48


          Implements comments from the pull request OpenEMS#2396

2fa57dd


          Change the Reactive Energy Channels from WATT_HOURS to CUMULATED_WATT…

…_HOURS

github-actions bot commented Nov 20, 2023

sfeilmeier changed the title ~~Feature/meter siemens pac1600~~ Implement Siemens PAC1600 meter

JosefRick added 2 commits

November 21, 2023 13:51


          Delete io.openems.edge.meter.siemens.pac1600/generated directory

9e123e8


          Update .gitignore

0a4842f

add 
/bin/
/bin_test/
/generated/

Contributor Author

JosefRick commented Nov 21, 2023

Thank you very much, Thomas, for your comments. The only thing I don't know is whether I can change the bundle with the Siemens PAC 2200 without crashing previous configurations. In principle I would think that would make sense, but the manufacturer is different.

JosefRick requested a review from tsicking

November 21, 2023 13:47

Contributor

sfeilmeier commented Nov 24, 2023

Thank you very much, Thomas, for your comments. The only thing I don't know is whether I can change the bundle with the Siemens PAC 2200 without crashing previous configurations. In principle I would think that would make sense, but the manufacturer is different.

The only thing relevant for a configuration is the Factory-ID, i.e. Meter.Siemens for the other implementation. As long as you don't touch that or use exactly the same for your implementation, there is no problem. It does not matter, if the Bundle of the Component changes - so no problem here from my side.

JosefRick added 3 commits

November 27, 2023 15:27


          Bundles the packages meter.siemens and meter.siemens.pac1600 into met…

803e7bb

…er.siemens


          deletes duplicate files

385d478


          Deletes the package io.openems.edge.meter.siemens.pac1600 from the Ed…

b865c92

…geApp

Contributor Author

JosefRick commented Nov 27, 2023

Thank you very much, Stefan, for your information on component identification. I have put the two packages together and hope that everything fits now.

tsicking requested changes

View reviewed changes

Contributor

tsicking left a comment

Thanks for the work and sorry for the delay. It looks much better to me now, though I still have some remarks regarding code style.

...penems.edge.meter.siemens/src/io/openems/edge/meter/siemens/pac1600/MeterSiemensPac1600.java Outdated

+              				.unit(Unit.MILLIVOLT)),
+              		REACTIVE_CONSUMPTION_ENERGY(Doc.of(OpenemsType.INTEGER) //
+              				.unit(Unit.CUMULATED_WATT_HOURS)),

Contributor

tsicking Dec 8, 2023

Strictly speaking the unit for these should be VOLT_AMPERE_REACTIVE_HOURS

...ms.edge.meter.siemens/src/io/openems/edge/meter/siemens/pac1600/MeterSiemensPac1600Impl.java

Contributor

tsicking Dec 8, 2023

This file should be formatted according to coding guidelines. See here: https://openems.github.io/openems.io/openems/latest/contribute/coding-guidelines.html#_tool_support

...ms.edge.meter.siemens/src/io/openems/edge/meter/siemens/pac1600/MeterSiemensPac1600Impl.java Outdated

+              					m(MeterSiemensPac1600.ChannelId.REACTIVE_CONSUMPTION_ENERGY_L3, new UnsignedDoublewordElement(6751),ElementToChannelConverter.DIRECT_1_TO_1),
+              					m(MeterSiemensPac1600.ChannelId.REACTIVE_PRODUCTION_ENERGY_L3, new UnsignedDoublewordElement(6753),ElementToChannelConverter.DIRECT_1_TO_1)));
+              		return modbusProtocol;

Contributor

tsicking Dec 8, 2023

You can make this "inline" (mark modbusProtocol and press Shift + Alt + I if you are using eclipse), then you don't need the local variable modbusProtocol. Your code will then simply look like this:
return new ModbusProtocol(this, // new FC3ReadRegistersTask(1, Priority.HIGH, // m(ElectricityMeter.ChannelId.VOLTAGE_L1, new UnsignedDoublewordElement(1), ElementToChannelConverter.SCALE_FACTOR_1), m(ElectricityMeter.ChannelId.VOLTAGE_L2, new UnsignedDoublewordElement(3), ElementToChannelConverter.SCALE_FACTOR_1), m(ElectricityMeter.ChannelId.VOLTAGE_L3, new UnsignedDoublewordElement(5), ElementToChannelConverter.SCALE_FACTOR_1), ...
barring formatting.

...s/src/io/openems/edge/meter/siemens/pac2200_3200_4200/MeterSiemensPac2200_3200_4200Impl.java Outdated

Contributor

tsicking Dec 8, 2023

I'm not sure about the naming with the underscore. As of now, this is not used anywhere else in OpenEMS as far as I can see, but I don't know a better naming either at the moment. Maybe something like MeterSiemensPacX200?
Same of course for the other related files.

Contributor Author

JosefRick Dec 8, 2023

I think that the name PacX200 is not good because there is also the Pac1200 and Pac5200 and these have a different Modbus protocol. Is the underscore really a problem. If not, I think calling it that is the best way to go.

JosefRick and others added 6 commits

December 8, 2023 18:09


          Changed the unit of reactive energy channels;

cdea833

Formatting of the code


          Update Bundle-Vendor

a6f0bd6


          Merge remote-tracking branch 'origin/develop' into feature/meterSieme…

6e9979c

…nsPAC1600


          Apply tools/prepare-commit.sh

e7839d7


          Review, Cleanup, Checkstyle

68a2275


          Organize imports + apply Eclipse autoformat

bc83b88

github-actions bot commented Jan 2, 2024

1 similar comment

github-actions bot commented Jan 2, 2024

sfeilmeier approved these changes

View reviewed changes

Contributor

sfeilmeier left a comment

Well done!

Thanks @tsicking for reviewing the code!

sfeilmeier merged commit 0dba9a2 into OpenEMS:develop

2 checks passed

ebram-tw pushed a commit to terawatt-infrastructure/openems that referenced this pull request


          Implement Siemens PAC1600 meter (OpenEMS#2396)

0cd0fcd

Co-authored-by: JosefRick <[email protected]>
Co-authored-by: Stefan Feilmeier <[email protected]>
Reviewed-by: Thomas Sicking <[email protected]>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet