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

Feature/667 Message payload C array types #673

Merged
merged 6 commits into from
May 14, 2024

Conversation

sassy-asjp
Copy link
Contributor

@sassy-asjp sassy-asjp commented Apr 17, 2024

Description

The wrapping for C arrays was modified to support more integer types, and fix incorrect handling of integers as floats for 2D arrays.

A message definition used only for unit tests was added.

Verification

Unit tests have been included.

Documentation

There is no documentation change required.

Future work

None.

@sassy-asjp sassy-asjp requested a review from a team as a code owner April 17, 2024 07:10
@sassy-asjp sassy-asjp force-pushed the feature/667-msg-array-types branch from 91a21c0 to 22454d9 Compare April 17, 2024 07:34
@sassy-asjp
Copy link
Contributor Author

Not sure why the Linux tests failed. I even tried inside docker Ubuntu 20.04 using pytest-xdist and could not reproduce locally

@schaubh
Copy link
Contributor

schaubh commented Apr 17, 2024

I'll pull this branch and see if it works on my macOS system. Don't have further info right now on the Linux CI failures.

@sassy-asjp
Copy link
Contributor Author

So I think the problem was in some situations (old python?? old swig??) the wrapping for the stdint fixed width types like uint64_t were guessed incorrectly. I was able to reproduce in Docker by trying to follow the GitHub action script line by line. Explicitly defining the wrappings for the stdint types fixed it.

I missed the failure originally since the build passed, however the problem was in build not test, just not reported correctly as #552

Copy link
Contributor

@juan-g-bonilla juan-g-bonilla left a comment

Choose a reason for hiding this comment

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

Thank you very much @sassy-asjp for this PR. I think it's exactly the right approach and I appreciate the nice test. However, I do have a couple suggestions to make the changes even better. These suggestions are not really about the code you've written, but about improving the macros wrt how they currently stand. It'd be great if you could take a look.

@sassy-asjp
Copy link
Contributor Author

@juan-g-bonilla I went through and thought about the python object reference counts and added some Py_DECREF where I thought appropriate.

@sassy-asjp sassy-asjp force-pushed the feature/667-msg-array-types branch from 34c3f6a to b01d2c3 Compare April 22, 2024 06:21
@juan-g-bonilla juan-g-bonilla self-requested a review April 23, 2024 03:38
Copy link
Contributor

@juan-g-bonilla juan-g-bonilla left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

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

thanks for the contribution. I pulled your branch and did a clean build and didn't encounter any issues with all our tests (including opNav) that are not tested on the CI.

Regarding closing this PR, could you please add a brief release notes statement in
basilisk/docs/source/Support/bskReleaseNotes.rst

You can add a bullet under the current beta release notes under the line

Version |release|
-----------------

Copy link
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

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

all variables need to have a doxygen comment string. Right now the documentation build throws lots of warnings.

src/architecture/msgPayloadDefC/TypesTestMsgPayload.h Outdated Show resolved Hide resolved
@patkenneally
Copy link
Contributor

If okay with others, I'd like a little more time to also review this PR.

@sassy-asjp sassy-asjp force-pushed the feature/667-msg-array-types branch from adceabe to bbebb5c Compare May 9, 2024 00:50
@patkenneally patkenneally self-requested a review May 13, 2024 19:31
@schaubh
Copy link
Contributor

schaubh commented May 13, 2024

@sassy-asjp , we are ready to push this branch to BSK develop. Another branch was pushed recently. Could you pease rebase and resolve any conflict in the release notes RST file? Thanks. I can then close this PR.

@sassy-asjp sassy-asjp force-pushed the feature/667-msg-array-types branch from bbebb5c to 6344944 Compare May 13, 2024 23:58
@sassy-asjp
Copy link
Contributor Author

@schaubh Done

@schaubh schaubh self-requested a review May 14, 2024 01:48
@schaubh schaubh merged commit c19f00e into AVSLab:develop May 14, 2024
2 checks passed
@sassy-asjp sassy-asjp deleted the feature/667-msg-array-types branch May 16, 2024 08:36
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.

Unable to access elements of custom message if they are an array of uint16/int16
5 participants