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

feat: add comparison operators to PhysicalNumber #997

Merged

Conversation

andreasWallner
Copy link
Collaborator

@andreasWallner andreasWallner commented Dec 12, 2022

Context, Motivation & Description

In code for checking frequencies/times are in range, e.g. for calculating&checking PLL parameters it's much more readable when PhysicalNumbers can be compared directly and with units, instead of having to go through toBigDecimal.

assert(fdiv_freq < 420MHz)
//vs
assert(fdiv_freq.toBigDecimal < 420000000L)

Impact on code generation

None

Checklist

  • Unit tests were added
  • API changes are or will be documented:

@kleinai
Copy link
Collaborator

kleinai commented Dec 12, 2022

I think we can merge this without issue if it passes tests.

@kleinai
Copy link
Collaborator

kleinai commented Dec 13, 2022

I'm going to fix the offending sim code because I wrote it two different ways and I prefer the match-case way instead of the not equal check.

@andreasWallner
Copy link
Collaborator Author

So... with this implementation != null works as required. Removing it was the right thing to do, since HertzNumber and TimeNumber are case classes and have a compiler generated, member equality checking, overridden equals anyways, so mine would never be called...

@numero-744
Copy link
Collaborator

How do you think it should be documented? (Do you think it should be?)

@numero-744 numero-744 changed the title Add comparison operators to PhysicalNumber feat: add comparison operators to PhysicalNumber Dec 18, 2022
@andreasWallner
Copy link
Collaborator Author

How do you think it should be documented? (Do you think it should be?)

I don´t think we need specific documentation, PhysicalUnit is only mentioned in a small subchapter in the documentation - if there is a user who needs the comparison I guess it will just feel naturally?

@numero-744 numero-744 merged commit f6f416f into SpinalHDL:dev Dec 18, 2022
@andreasWallner andreasWallner deleted the physical_comparison_operators branch December 18, 2022 21:15
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.

None yet

3 participants