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 FB_AtomicNumber and FB_AtomicNumber_Test. #9

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

NSLentz
Copy link
Contributor

@NSLentz NSLentz commented Sep 26, 2024

  1. Added FB_AtomicNumber as generated from tc3-table-generator except changed datatype to INT.
  2. Added FB_AtomicNumber_Test extending TcUnit.FB_TestSuite.

Motivation and Context

  • The atomic number is needed for calculations of focal length. I am adding a simple atomic number lookup function block based on periodictable.

How Has This Been Tested?

  1. Checked to make sure the library builds successfully.
  2. Added two unit tests for the added function block. They check to make sure the bFound bit works properly and that we get back the correct atomic numbers for a sampling of elements.
  3. Visual spot check was performed comparing information from ptable to the lookup file to make sure the atomic numbers are correct.

Where Has This Been Documented?

  • The README was updated to include mention of the atomic number lookup functionality.

Pre-merge checklist

  • Code works interactively
  • Code contains descriptive comments
  • Test suite passes locally
  • Libraries are set to Always Newest version (Library, *)
  • Committed with pre-commit or ran pre-commit run --all-files

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

Addition and tests look reasonable, no merge-blocking notes.

Tiny design question: you switched these from LREAL to INT. Did you consider UINT? Ultimately this doesn't matter at all ever because atomic numbers don't go up very high (we could have even used an USINT) and there isn't much to gain from noodling on it too much.

@NSLentz
Copy link
Contributor Author

NSLentz commented Sep 27, 2024

@ZLLentz
I considered a UINT, but then recalled how TwinCAT handles math between UINTs and other INTs. It quietly returns a UINT, which has led me to bugs when math between a UINT and an INT returns a negative number. Since atomic numbers will likely never reach the INT limit, I preferred to go for an INT to eliminate the mistake of trying to do math in TwinCAT with UINTs and INTS.

@NSLentz NSLentz merged commit 07e59a3 into pcdshub:master Sep 27, 2024
9 checks passed
@NSLentz NSLentz deleted the add-atomic-number branch September 27, 2024 19:59
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