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

Added real32 (Beckhoff float) and real64 (Beckhoff double) #132

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

JensVanhooydonck
Copy link
Contributor

@JensVanhooydonck
Copy link
Contributor Author

Still need to test this. As soon as next monday.

@JensVanhooydonck JensVanhooydonck marked this pull request as draft June 25, 2024 14:30
@yguel
Copy link
Contributor

yguel commented Jun 25, 2024

Hi JensVanhooydonck,
Again, many thanks for all your PR.
All the checks did not pass, but the code sounds good and I agree with the need of the feature.
As soon as the checks pass, I will merge the PR.

@JensVanhooydonck
Copy link
Contributor Author

It seems that the EC_READ_REAL and EC_WRITE_REAL funtions aren't always available. Used the same logic (bit casting) and reading and writing with the EC_READ_UINT32 and EC_WRITE_UINT32 functions.

For the sdo, there was currently only int value data available. I added a double data value as well. Should I keep it this way, or should I change the data type from int to double and cast the value to int for the existing sdo's?

Also, should I also add the double/real64 values as well?

@JensVanhooydonck JensVanhooydonck changed the title Added real32 (Beckhoff float) Added real32 (Beckhoff float) and real64 (Beckhoff double) Jul 2, 2024
@JensVanhooydonck JensVanhooydonck marked this pull request as ready for review July 2, 2024 09:52
@yguel
Copy link
Contributor

yguel commented Jul 4, 2024

Hi JensVanhooydonck,

It seems that the EC_READ_REAL and EC_WRITE_REAL functions aren't always available. Used the same logic (bit casting) and reading and writing with the EC_READ_UINT32 and EC_WRITE_UINT32 functions.

That is the way it should be done to work everywhere.

For the sdo, there was currently only int value data available. I added a double data value as well. Should I keep it this way, or should I change the data type from int to double and cast the value to int for the existing sdo's?

I think having different types is the ideal way to go, especially if we imagine having 64 bits integers at some point.

Also, should I also add the double/real64 values as well?

2 cases:

  • if there is no real use case, I prefer to wait for one and document how it could be done somewhere, to not work for nothing. But
  • if it is very very easy, like it takes you less than 5min, then it is the best way to document it, so let's do it.

@JensVanhooydonck
Copy link
Contributor Author

Hi JensVanhooydonck,

It seems that the EC_READ_REAL and EC_WRITE_REAL functions aren't always available. Used the same logic (bit casting) and reading and writing with the EC_READ_UINT32 and EC_WRITE_UINT32 functions.

That is the way it should be done to work everywhere.

For the sdo, there was currently only int value data available. I added a double data value as well. Should I keep it this way, or should I change the data type from int to double and cast the value to int for the existing sdo's?

I think having different types is the ideal way to go, especially if we imagine having 64 bits integers at some point.

Also, should I also add the double/real64 values as well?

2 cases:

  • if there is no real use case, I prefer to wait for one and document how it could be done somewhere, to not work for nothing. But
  • if it is very very easy, like it takes you less than 5min, then it is the best way to document it, so let's do it.
  1. I've added all logic (casting to U32 and U64) because the build in functions didn't work for me. (Gave a runtime error, building was ok). Didn't figure out why it didn't find the functions.

  2. Currently i've altered the code so when setting the sdo values, this is already casted to the int data value. If we need it in the future to also support 64-bit ints we need to fix it then i assume.

  3. For double/real64 in PDO i've added the code already.

I've tested this with real32 already and everything seems to work correctly. Don't have a device with Real64 items currently. If needed, i can comment this part out.

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.

2 participants