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

Create a VersionChild datatype #123

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Create a VersionChild datatype #123

merged 1 commit into from
Jul 20, 2023

Conversation

gerw
Copy link
Collaborator

@gerw gerw commented Jul 16, 2023

This works towards #122.

In calculations.py we have an exceptional handling for the fields 81-90, since these ten fields contain the version information. Positions 82-90 were uninhabited; with this patch, a new VersionChild sits there and gobbles the value for its parent.

A new flag datatype_internal indicates, that this datatype is only internal and the iterator of Calculations will skip this entry.

@classmethod
def from_heatpump(cls, value):
return "".join([chr(c) for c in value]).strip("\x00")

@Base.value.getter
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pylint complains here. Is this the correct way to overwrite the getter of value in the Base class?

Comment on lines 126 to 135
81: version,
82: version.spawn_child(),
83: version.spawn_child(),
84: version.spawn_child(),
85: version.spawn_child(),
86: version.spawn_child(),
87: version.spawn_child(),
88: version.spawn_child(),
89: version.spawn_child(),
90: version.spawn_child(),
Copy link
Collaborator

@kbabioch kbabioch Jul 17, 2023

Choose a reason for hiding this comment

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

  • Does this datatype no longer have a name (ID_WEB_SoftStand)?
  • I totally get that the handling of this datatype was not particularly nice and iterating over it was generating confusing results. While this fixes it, the definition doesn't look super handy. Couldn't we address this in the iterator itself somehow by indicating the length of each datatype, rather than specifying an internal flag 10 times iteratively?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • The name is unchanged, it is defined in Line 43.
  • The version datatype is the only one with a length bigger than 1. Thus, it was more reasonable to me to handle it as an exceptional case by introducing the "sub"-datatype.
  • To which "iterator" are you referring to?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The name is unchanged, it is defined in Line 43.

Ah, somehow missed that on first glance, sorry for that.

The version datatype is the only one with a length bigger than 1. Thus, it was more reasonable to me to handle it as an exceptional case by introducing the "sub"-datatype.

Depending on how we're going to tackle #33, we might have other datatypes with length > 1, also.

Personally I would prefer some kind of length attribute (with the default being 1), compared to this approach.

To which "iterator" are you referring to?

In the description of this pull request you were referring to the iterator. I suppose you mean the __iter__ method of Calculations. Haven't checked this in close detail yet, probably it didn't need to skip anything before, since the values have not been considered for iteration in the first place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Concerning #33: In my opinion, the data there has length one (each time sits in one slot). The overall handling of the data there should be (in my opinion) handled in a higher-level way in the sense of #119.

@kbabioch
Copy link
Collaborator

There seem to be two issues with pylint (as already noted).

 luxtronik/datatypes.py:354:4: W0236: Method 'value' was expected to be 'property', found it instead as 'method' (invalid-overridden-method)
************* Module tests.test_datatypes
tests/test_datatypes.py:713:15: W0143: Comparing against a callable, did you omit the parenthesis? (comparison-with-callable)

@kbabioch
Copy link
Collaborator

Other input from @Bouni et al. might also be welcome ;-).

@gerw
Copy link
Collaborator Author

gerw commented Jul 17, 2023

There seem to be two issues with pylint (as already noted).

 luxtronik/datatypes.py:354:4: W0236: Method 'value' was expected to be 'property', found it instead as 'method' (invalid-overridden-method)
************* Module tests.test_datatypes
tests/test_datatypes.py:713:15: W0143: Comparing against a callable, did you omit the parenthesis? (comparison-with-callable)

Yes, but I do not understand how to fix it.

@gerw gerw changed the title Create a Version datatype Create a VersionChild datatype Jul 17, 2023
@gerw
Copy link
Collaborator Author

gerw commented Jul 19, 2023

I realized another possibility which seems to (almost) remove the exceptional handling:

  • We introduce a new datatype character which just converts 1 raw value in the corresponding character.
  • Parameters 81-90 get this new datatype (and they will store one character of the version string)
  • The version string is provided by the new higher-level interface from Provide high level interface #119

@Bouni
Copy link
Owner

Bouni commented Jul 19, 2023

@gerw That seems like the right way to solve this to me 👍🏽

@kbabioch
Copy link
Collaborator

kbabioch commented Jul 19, 2023

I realized another possibility which seems to (almost) remove the exceptional handling:

  • We introduce a new datatype character which just converts 1 raw value in the corresponding character.
  • Parameters 81-90 get this new datatype (and they will store one character of the version string)
  • The version string is provided by the new higher-level interface from Provide high level interface #119

That sounds also more "keep it simple"-like to me and introduces less complexity in the interpretation of the raw data by the current datatypes.

So I would definitely prefer that.

@gerw
Copy link
Collaborator Author

gerw commented Jul 19, 2023

@Bouni @kbabioch Done.

Copy link
Collaborator

@kbabioch kbabioch left a comment

Choose a reason for hiding this comment

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

Perfect, looks good to me.

@Bouni Bouni merged commit 8e37670 into Bouni:main Jul 20, 2023
@gerw gerw mentioned this pull request Jul 21, 2023
@gerw gerw deleted the version_datatype branch February 5, 2024 13:47
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