Skip to content

[fix] modm::Saturated #780

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 14, 2021
Merged

[fix] modm::Saturated #780

merged 1 commit into from
Dec 14, 2021

Conversation

@TomSaw
Copy link
Contributor

@TomSaw TomSaw commented Nov 15, 2021 •
edited
Loading

To PR the generic colors nextly from #665 , i would like to PR this fixed modm::Saturated first.

TODO

  • Add missing operator+= / -=
  • Extend tests to more types and more cases

@TomSaw TomSaw force-pushed the fix/math-saturation branch from 473cefa to 6c25b15 Compare November 15, 2021 12:07
@TomSaw TomSaw changed the title fixed modm::Saturated [fixed modm::Saturated Nov 15, 2021
@TomSaw TomSaw changed the title [fixed modm::Saturated [fix] modm::Saturated Nov 15, 2021
@TomSaw TomSaw force-pushed the fix/math-saturation branch from 6c25b15 to 9d0c642 Compare November 15, 2021 12:09
@TomSaw TomSaw mentioned this pull request Nov 15, 2021
Open
4 tasks
salkinium
salkinium reviewed Nov 15, 2021
View reviewed changes
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Nice!

@TomSaw TomSaw force-pushed the fix/math-saturation branch 2 times, most recently from c358128 to 8bfb47a Compare November 23, 2021 08:07
@TomSaw TomSaw force-pushed the fix/math-saturation branch from 8bfb47a to d21beb8 Compare December 12, 2021 11:05
@TomSaw
Copy link
Contributor Author

TomSaw commented Dec 12, 2021

Done! Please review @salkinium

salkinium
salkinium approved these changes Dec 12, 2021
View reviewed changes
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Lovely!

@TomSaw TomSaw force-pushed the fix/math-saturation branch 16 times, most recently from fba61f5 to c9d4d44 Compare December 13, 2021 07:15
@TomSaw
Copy link
Contributor Author

TomSaw commented Dec 13, 2021

Could be merged @salkinium 😋

@TomSaw
Copy link
Contributor Author

TomSaw commented Dec 13, 2021

... still in exams phase?

@salkinium
Copy link
Member

salkinium commented Dec 13, 2021 •
edited
Loading

... still in exams phase?

Way worse: master thesis phase for the next ~6 months.

@salkinium
Copy link
Member

salkinium commented Dec 13, 2021

Could be merged @salkinium 😋

Rebase and squash?

@TomSaw TomSaw force-pushed the fix/math-saturation branch from c9d4d44 to 64db86f Compare December 13, 2021 20:55
@TomSaw
Copy link
Contributor Author

TomSaw commented Dec 13, 2021 •
edited
Loading

Way worse: master thesis phase for the next ~6 months.

So i stay in @-poke-mode for ~6 months @salkinium 😬 !? Hope you find a breath in between.

@salkinium
Copy link
Member

salkinium commented Dec 14, 2021

So i stay in @-poke-mode for ~6 months @salkinium 😬 !?

You should probably always stay in poke mode, since you push so many commits that I don't really know when do reviews or merges 😜 So it's best if you just tell me, that helps me and you.

@TomSaw TomSaw force-pushed the fix/math-saturation branch from 53a15f6 to c0a8c51 Compare December 14, 2021 09:02
@salkinium salkinium merged commit c0a8c51 into modm-io:develop Dec 14, 2021
@TomSaw
Copy link
Contributor Author

TomSaw commented Dec 14, 2021 •
edited
Loading

Thanks for the feedback, i thought like pushing commits that hard isn't normal. I rely to much on githubs CI and it always surprises me - repeating - what else got broken with my changes.

I can run anything within '/test' but there is more! Running githubs complete CI locally would save a ton of nerves and CO2. Is there some 'best practice' to read? @salkinium

@salkinium
Copy link
Member

salkinium commented Dec 14, 2021

See this Readme here: https://github.com/modm-io/modm/tree/develop/test#unit-tests

@salkinium
Copy link
Member

salkinium commented Dec 14, 2021

There are also a bunch of scripts in tools/scripts, for the usage consult the GitHub CI runner: https://github.com/modm-io/modm/blob/develop/.github/workflows/linux.yml#L32-L62

@salkinium
Copy link
Member

salkinium commented Dec 14, 2021

The compile-all tests take a very long time locally, I suggest only running them only locally for a subset of devices if the ci:hal label has failed.
By calling it manually you can filter the failing devices by their prefix python3 run_all.py $PREFIX which reduced the scope significantly.
There are also ignored devices, but they are centrally handled by repo.lb: https://github.com/modm-io/modm/blob/develop/test/all/ignored.txt

@TomSaw TomSaw deleted the fix/math-saturation branch December 14, 2021 16:32
@salkinium salkinium added this to the 2021q4 milestone Dec 18, 2021
@TomSaw
Copy link
Contributor Author

TomSaw commented Jan 11, 2022

Perfect. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@salkinium salkinium salkinium approved these changes

Assignees

No one assigned

Labels

enhancement 🌈 feature 🚧

Milestone

2021q4

Development

Successfully merging this pull request may close these issues.

2 participants

@TomSaw @salkinium