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

Add int64in/out record support #161

Merged
merged 5 commits into from
Jun 7, 2024
Merged

Conversation

AlexanderWells-diamond
Copy link
Collaborator

@AlexanderWells-diamond AlexanderWells-diamond commented Jun 6, 2024

Add support for the int64in and int64out record types.

There's two TODOs and some test failures that needs discussion:

  • The DBR_* types we use come from epicscorelibs, however its latest version does not define the (new) DBR_INT64 and DBR_UINT64 types. Is it worth trying to get a PR through that module that adds these codes before merging this?
  • I don't fully understand the note next to DBF_ULONG as to why it is an int32, rather than uint32. Will I need to do the same kind of transformation for DBF_UINT64?
  • There are a variety of test failures due to caget not being able to transport an int64, instead transporting it as a float. Should I special-case these tests to accept this, or is there a better mechanism to force what we want? I see that caget does NOT claim to support datatype=DBR_INT64.

Various reference materials:
Changelog for introduction of the records: https://github.com/epics-base/epics-base/blob/7.0/documentation/RELEASE_NOTES.md#ioc-database-support-for-64-bit-integers
EPICS header file that defines both DBR_ and DBF_ codes: https://github.com/epics-base/epics-base/blob/7.0/modules/database/src/ioc/dbStatic/dbFldTypes.h

Closes #160 Closes #118

Copy link

github-actions bot commented Jun 6, 2024

Unit Test Results

       8 files  ±    0         8 suites  ±0   10m 12s ⏱️ +18s
   297 tests +  13     281 ✔️ +13    16 💤 ±  0  0 ±0 
2 376 runs  +104  1 928 ✔️ +72  448 💤 +32  0 ±0 

Results for commit d38df9b. ± Comparison against base commit 4bbc933.

This pull request removes 14 and adds 27 tests. Note that renamed tests count towards both.
tests.test_record_values.TestDefaultValue ‑ test_value_default_pre_init[WaveformIn-expected_value12-ndarray]
tests.test_record_values.TestDefaultValue ‑ test_value_default_pre_init[WaveformOut-expected_value11-ndarray]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[WaveformIn-bytes-[72 69 76 76 79  0 87 79 82 76 68]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[WaveformIn-float-[12.345]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[WaveformIn-int-[567.]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[WaveformOut-bytes-[72 69 76 76 79  0 87 79 82 76 68]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[WaveformOut-float-[12.345]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[WaveformOut-int-[567.]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_set[WaveformIn-bytes-[72 69 76 76 79  0 87 79 82 76 68]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_set[WaveformIn-float-[12.345]]
…
tests.test_record_values.TestDefaultValue ‑ test_value_default_pre_init[WaveformIn-expected_value14-ndarray]
tests.test_record_values.TestDefaultValue ‑ test_value_default_pre_init[WaveformOut-expected_value13-ndarray]
tests.test_record_values.TestDefaultValue ‑ test_value_default_pre_init[int64In-0-int]
tests.test_record_values.TestDefaultValue ‑ test_value_default_pre_init[int64Out-0-int]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[WaveformIn-list-[72 69 76 76 79  0 87 79 82 76 68]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[WaveformOut-list-[72 69 76 76 79  0 87 79 82 76 68]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[int64In-int-65]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_initial_value[int64Out-int-65]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_set[WaveformIn-list-[72 69 76 76 79  0 87 79 82 76 68]]
tests.test_record_values.TestGetValue ‑ test_value_pre_init_set[WaveformOut-list-[72 69 76 76 79  0 87 79 82 76 68]]
…

♻️ This comment has been updated with latest results.

@coretl
Copy link
Contributor

coretl commented Jun 6, 2024

There are a variety of test failures due to caget not being able to transport an int64, instead transporting it as a float. Should I special-case these tests to accept this, or is there a better mechanism to force what we want? I see that caget does NOT claim to support datatype=DBR_INT64.

pvget should support it though, so you can use p4p to do those particular tests

Copy link
Collaborator

@Araneidae Araneidae left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Are we going to have to add these field types to cothread and aioca?

docs/reference/api.rst Show resolved Hide resolved
softioc/fields.py Outdated Show resolved Hide resolved
softioc/fields.py Outdated Show resolved Hide resolved
It is necessary as ChannelAccess does not support int64 natively,
instead transferring it using a double. This means the value returned
from caget is actually a float, which is inconvenient.
Fortunately p4p (using PVAccess) does support int64!

I deleted some test cases as they were really testing the behaviour of
caget/caput, and not pythonSoftIoc itself.
@AlexanderWells-diamond
Copy link
Collaborator Author

Requested changes have been made. Tests are failing due to #158 still waiting for upstream dependencies for Python 3.12.

Copy link
Collaborator

@Araneidae Araneidae left a comment

Choose a reason for hiding this comment

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

This is fine.

@AlexanderWells-diamond AlexanderWells-diamond merged commit d38df9b into master Jun 7, 2024
26 of 30 checks passed
@AlexanderWells-diamond AlexanderWells-diamond deleted the add_int64_records branch June 7, 2024 13:02
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.

Allow int64 datatype Support new EPICS 7 records
3 participants