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

--[BE] - SensorAttributes/Managers refactor Part 1 #2502

Merged
merged 43 commits into from
Dec 9, 2024

Conversation

jturner65
Copy link
Contributor

@jturner65 jturner65 commented Nov 14, 2024

Motivation and Context

This PR is the first part of the Sensor refactor effort, responsible for enabling the creation of Sensor-type-specific Attributes from provided SensorSpecs, such that the attributes themselves will be used to create the Sensors (which will be implemented in Part 2).
Specifically, this PR provides the following :

  1. An Attributes hierarchy mirroring the SensorSpec hierarchy
  2. A SensorAttributesManager that will manage these SensorAttributes and provide access to them, including a method for creating the appropriate Attributes from a given SensorSpec.
  3. Tests to verify the appropriate creation and initialization of a particular Attributes from its equivalent SensorSpec.

How Has This Been Tested

Locally c++ tests pass. New tests were added to test the valid creation and data content of Attributes from SensorSpecs.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Nov 14, 2024
@jturner65 jturner65 requested a review from aclegg3 November 14, 2024 14:53
@jturner65 jturner65 force-pushed the BE_SensorMetadataRefactor branch 3 times, most recently from 6eab72b to b22de05 Compare November 20, 2024 19:05
@jturner65 jturner65 force-pushed the BE_SensorMetadataRefactor branch 3 times, most recently from 52b14cc to a2bbbe5 Compare November 25, 2024 21:24
@jturner65 jturner65 force-pushed the BE_SensorMetadataRefactor branch from 99e44bc to 6b449eb Compare December 4, 2024 13:55
@jturner65 jturner65 marked this pull request as ready for review December 5, 2024 18:57
@jturner65 jturner65 force-pushed the BE_SensorMetadataRefactor branch 3 times, most recently from d9d788a to a471ac3 Compare December 8, 2024 18:33
@jturner65 jturner65 changed the title --[BE][WIP] - SensorAttributes/Managers refactor --[BE] - SensorAttributes/Managers refactor Part 1 Dec 9, 2024
@jturner65 jturner65 force-pushed the BE_SensorMetadataRefactor branch from cab7613 to 29f0d22 Compare December 9, 2024 17:49
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Looks like a good step toward the planned design. 👍
Added some thoughts about next steps mostly.

src/esp/metadata/attributes/CubeMapSensorAttributes.cpp Outdated Show resolved Hide resolved
Comment on lines +87 to +89
// TODO : Rename attributes to appropriate name and register
// Build name using more than just spec uuid?
std::string newAttrHandle = sensorSpec->uuid;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably need some de-duplication system for unique naming. Spec will go away, so this doesn't need to be tied to that field. Naming could be a part of registering if it doesn't come from a filename for example.

Copy link
Contributor Author

@jturner65 jturner65 Dec 9, 2024

Choose a reason for hiding this comment

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

Yeah, postCreateRegister calls preRegisterObjectFinalize below, which does this (makes sure the name is unique - uuid becomes uuid_XXXX where XXXX is a unique number.

void SensorAttributesManager::setValsFromJSONDoc(
AbstractSensorAttributes::ptr attribs,
const io::JsonGenericValue& jsonConfig) {
// TODO support loading values from JSON docs for each type of
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to have 1 default for each sensor type, maybe in test_assets? Or programmatically created and added like prims.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I planned on doing that in the 2nd PR.

@jturner65 jturner65 merged commit f3089bc into main Dec 9, 2024
7 checks passed
@jturner65 jturner65 deleted the BE_SensorMetadataRefactor branch December 9, 2024 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants