Skip to content
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

Bug in Edge2Edge in ModbusType Float64 fixed. #2265

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

tsicking
Copy link
Contributor

Fixed Typo: Changed UnsignedQuadrupleWordElement to FloatQuadrupleWordElement.

@github-actions
Copy link

Code Coverage

Copy link
Contributor

@clehne clehne left a comment

Choose a reason for hiding this comment

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

Ok.

@huseyinsaht
Copy link
Contributor

Thanks a lot for this bug fix.
I have a wish here;
Can you please use new java 17 features here; in that case for switch case format like;
return switch (type) {
case UINT16, ENUM16 -> new UnsignedWordElement(address);
case UINT32 -> new UnsignedDoublewordElement(address);
case FLOAT32 -> new FloatDoublewordElement(address);
case FLOAT64 -> new UnsignedQuadruplewordElement(address);
case STRING16 -> new StringWordElement(address, 16);
};
Thanks a lot.

@tsicking
Copy link
Contributor Author

Hi Huseyin, thanks for your comment. I have done the needful :-)

@github-actions
Copy link

Code Coverage

@huseyinsaht
Copy link
Contributor

yep I have seen it.
One return before switch case is enough and in my opinion we dont need this curly braces anymore. and we can use the default to return null back.
Can you please check it again.

I would so write it;

private static AbstractModbusElement<?> generateModbusElement(ModbusType type, int address) {
	return switch (type) {
	case UINT16, ENUM16 -> new UnsignedWordElement(address);
	case UINT32 -> new UnsignedDoublewordElement(address);
	case FLOAT32 -> new FloatDoublewordElement(address);
	case FLOAT64 -> new FloatQuadruplewordElement(address);
	case STRING16 -> new StringWordElement(address, 16);
	default -> null;
	};
}

@tsicking
Copy link
Contributor Author

yep I have seen it. One return before switch case is enough and in my opinion we dont need this curly braces anymore. and we can use the default to return null back. Can you please check it again.

I would so write it;

private static AbstractModbusElement<?> generateModbusElement(ModbusType type, int address) {
	return switch (type) {
	case UINT16, ENUM16 -> new UnsignedWordElement(address);
	case UINT32 -> new UnsignedDoublewordElement(address);
	case FLOAT32 -> new FloatDoublewordElement(address);
	case FLOAT64 -> new FloatQuadruplewordElement(address);
	case STRING16 -> new StringWordElement(address, 16);
	default -> null;
	};
}

Oh, I see. I will update in a minute. Haven't got used to the new switch-case format.

@huseyinsaht
Copy link
Contributor

yep I have seen it. One return before switch case is enough and in my opinion we dont need this curly braces anymore. and we can use the default to return null back. Can you please check it again.
I would so write it;

private static AbstractModbusElement<?> generateModbusElement(ModbusType type, int address) {
	return switch (type) {
	case UINT16, ENUM16 -> new UnsignedWordElement(address);
	case UINT32 -> new UnsignedDoublewordElement(address);
	case FLOAT32 -> new FloatDoublewordElement(address);
	case FLOAT64 -> new FloatQuadruplewordElement(address);
	case STRING16 -> new StringWordElement(address, 16);
	default -> null;
	};
}

Oh, I see. I will update in a minute. Haven't got used to the new switch-case format.

👍🏻 Thanks a lot. I ll approve and inform Stefan.

@github-actions
Copy link

Code Coverage

@huseyinsaht huseyinsaht requested a review from sfeilmeier July 12, 2023 14:59
# Conflicts:
#	io.openems.edge.edge2edge/src/io/openems/edge/edge2edge/common/AbstractEdge2Edge.java
@tsicking tsicking requested a review from clehne January 23, 2025 12:05
Copy link

codecov bot commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2265      +/-   ##
=============================================
- Coverage      57.03%   57.00%   -0.02%     
+ Complexity      9810     9800      -10     
=============================================
  Files           2279     2279              
  Lines          97390    97390              
  Branches        7055     7055              
=============================================
- Hits           55538    55512      -26     
- Misses         39809    39833      +24     
- Partials        2043     2045       +2     

@clehne
Copy link
Contributor

clehne commented Jan 24, 2025

@sfeilmeier This PR is pretty old. Is there a specific reason for not merging it or did iit get lost in the crowd of PRs?
Without this the Edge2EdgeEss will not work.
If there is no specific reason we would add the missing Junit tests in the next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants