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

Diode test card OD #36

Merged
merged 8 commits into from
Aug 12, 2024
Merged

Diode test card OD #36

merged 8 commits into from
Aug 12, 2024

Conversation

chroco
Copy link
Contributor

@chroco chroco commented Jul 21, 2024

These are the updates to the OD for the diode test card.

@chroco chroco requested a review from ryanpdx July 21, 2024 20:57
@ThirteenFish ThirteenFish self-requested a review July 21, 2024 21:18
Copy link
Contributor

@ThirteenFish ThirteenFish left a comment

Choose a reason for hiding this comment

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

I'd like to see more usage of the descriptive fields where applicable. These include:

  • unit and scale_factor. Prefer standard units like A or S and then use scale_factor to scale them to the actual value you put in the subindex. For example if you report led_current in uA, then set uint to A and scale_factor to 0.000001.
  • high_limit and low_limit. Only if the limits are readily available, otherwise the whole range is valid.
  • value_descriptions. For enum-like things. Probably ctrl?
  • bit_definitions. For bitfield like things. Probably error and status?
  • default. Where a default value would make sense. Maybe ctrl and mux_select?

You can find some documentation here but there's also examples in other configs on how to use them.

@chroco
Copy link
Contributor Author

chroco commented Jul 23, 2024

Thanks Theo! I'll work on these changes. I think I am blocked on the units and scale_factor in the adcsample portion of the OD until I have more information from Anna and Andrew. That's why this is left as uint. This portion of the code is unfinished in app_diode as well as the bit fields in error and status. I also don't expect all the bits to be used at this time. Do you want me to put placeholders in for the unknowns?

@ThirteenFish
Copy link
Contributor

I think I am blocked on the units and scale_factor in the adcsample portion of the OD until I have more information from Anna and Andrew.

If it's unknown it's fine to leave the units off until they're determined. An alternative idea, since this is ADC output, is to mark the unit as lsb for least significant bits. That's kinda the generic "here's an ADC" unit. Once the real unit is determined we can update the config.

This portion of the code is unfinished in app_diode as well as the bit fields in error and status. I also don't expect all the bits to be used at this time. Do you want me to put placeholders in for the unknowns?

No need for placeholders, only mark the bits that you use. As more bits get used the configs can be updated. If none are currently used you could possibly add a comment saying "currently unused" or something to that effect.

oresat_configs/base/diode_test.yaml Outdated Show resolved Hide resolved
oresat_configs/base/diode_test.yaml Outdated Show resolved Hide resolved
@ThirteenFish ThirteenFish merged commit d392897 into master Aug 12, 2024
1 check passed
@ThirteenFish ThirteenFish deleted the diode_test branch August 12, 2024 00:27
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