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.

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

sys/color: extend unittest and fix module #19693

Merged
merged 2 commits into from
Jun 6, 2023
Merged

sys/color: extend unittest and fix module #19693

merged 2 commits into from
Jun 6, 2023

Conversation

kfessel
Copy link
Contributor

@kfessel kfessel commented May 31, 2023 •
edited
Loading

Contribution description

this extends the unittest for sys_color testing more colors

Testing procedure

RIOT_tree/tests/unittests$ make tests-color test

will fail since our rgb2hsv implementation is wrong (or is using an other colorspace than hsv2rgb (without documenting))

the new hsv2rgb test will succeed

Issues/PRs references

#19614 was the reason i had a look at this

#1315 added the rgb2hsv and hsv2rgb function

#9940 added the test for black special case

https://www.vagrearg.org/content/hsvrgb << some optimization for that function (avoiding float)

@kfessel kfessel requested a review from miri64 as a code owner May 31, 2023 15:54
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label May 31, 2023
@miri64
Copy link
Member

miri64 commented May 31, 2023

For reference, see #9940

@miri64
Copy link
Member

miri64 commented May 31, 2023

(not sure what you mean by "wrong" so I provided the black-corner case thingy for documentation)

@kfessel
Copy link
Contributor Author

kfessel commented May 31, 2023 •
edited
Loading

The test previously tested only one value (black) (for good reason).
I extended the test to test more values (using one of the very similar common tables of typical values that has some hsv an rgb value examples) .
Sadly #9939 does not fix fix enough of that function (maybe there is an undocumented color space adjustment that is not very common in that function (but the hsv2rgb the common color space so i assume the difference in rgb2hsv is an error).

@github-actions github-actions bot added the Area: sys Area: System label May 31, 2023
@kfessel kfessel changed the title unittest/sys_color: extend test sys/color: fix and extend unittest May 31, 2023
@kfessel kfessel requested a review from benpicco May 31, 2023 18:51
@kfessel kfessel changed the title sys/color: fix and extend unittest sys/color: extend unittest and fix module May 31, 2023
@kfessel
Copy link
Contributor Author

kfessel commented Jun 1, 2023 

color_rgb_t rgb;
color_hsv_t hsv;
color_rgb2hsv(&rgb, &hsv);
color_rgb_t rgb_z;
color_hsv2rgb(&hsv, &rgb_z)
rgb_z ==[+-1]  rgb ; //

is false with current master for many color values ; and true with this PR; ("black" ist working before and after)

maribu
maribu reviewed Jun 1, 2023
View reviewed changes
sys/color/color.c Show resolved Hide resolved
sys/color/color.c Outdated Show resolved Hide resolved
sys/color/color.c Outdated Show resolved Hide resolved
sys/color/color.c Outdated Show resolved Hide resolved
sys/color/color.c Outdated Show resolved Hide resolved
sys/color/color.c Outdated Show resolved Hide resolved
tests/unittests/tests-color/tests-color.c Outdated Show resolved Hide resolved
@kfessel kfessel added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 5, 2023
@riot-ci
Copy link

riot-ci commented Jun 5, 2023 •
edited
Loading

Murdock results

✔️ PASSED

19a194c sys/color: fix rgb2hsv function

Success Failures Total Runtime
6933 0 6933 12m:27s

Artifacts

maribu
maribu approved these changes Jun 5, 2023
View reviewed changes
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

The code looks good to me and decent test coverage was added.

Please squash the minor style nitpicks directly; the ACK remains valid.

sys/color/color.c Outdated Show resolved Hide resolved
sys/color/color.c Outdated Show resolved Hide resolved
sys/color/color.c Outdated Show resolved Hide resolved
sys/color/color.c Show resolved Hide resolved
tests/unittests/tests-color/tests-color.c Outdated Show resolved Hide resolved
tests/unittests/tests-color/tests-color.c Outdated Show resolved Hide resolved
tests/unittests/tests-color/tests-color.c Outdated Show resolved Hide resolved
tests/unittests/tests-color/tests-color.c Outdated Show resolved Hide resolved
@kfessel
Copy link
Contributor Author

kfessel commented Jun 5, 2023

bors merge

bors bot added a commit that referenced this pull request Jun 5, 2023
@bors @kfessel
Merge #19693
5dc58ad 
19693: sys/color: extend unittest and fix module r=kfessel a=kfessel

### Contribution description

this extends the unittest for sys_color testing more colors 

### Testing procedure

```
RIOT_tree/tests/unittests$ make tests-color test
```
will fail since our `rgb2hsv` implementation is wrong (or is using an other colorspace than hsv2rgb (without documenting))

the new `hsv2rgb` test will succeed

### Issues/PRs references

#19614  was the reason i had a look at this 

#1315 added the rgb2hsv and hsv2rgb function

#9940 added the test for black special case

https://www.vagrearg.org/content/hsvrgb << some optimization for that function (avoiding float)


Co-authored-by: Karl Fessel <karl.fessel@ovgu.de>
@kfessel
Copy link
Contributor Author

kfessel commented Jun 5, 2023

bors retry

@bors
Copy link
Contributor

bors bot commented Jun 5, 2023

Already running a review

@kfessel
Copy link
Contributor Author

kfessel commented Jun 5, 2023

bors cancel
bors merge

@bors
Copy link
Contributor

bors bot commented Jun 5, 2023

Canceled.

bors bot added a commit that referenced this pull request Jun 5, 2023
@bors @benpicco @kfessel
Merge #19378 #19693
8b6570d 
19378: sys: add common imath module mv isin() form test/driver_dac_dds r=kfessel a=benpicco



19693: sys/color: extend unittest and fix module r=kfessel a=kfessel

### Contribution description

this extends the unittest for sys_color testing more colors 

### Testing procedure

```
RIOT_tree/tests/unittests$ make tests-color test
```
will fail since our `rgb2hsv` implementation is wrong (or is using an other colorspace than hsv2rgb (without documenting))

the new `hsv2rgb` test will succeed

### Issues/PRs references

#19614  was the reason i had a look at this 

#1315 added the rgb2hsv and hsv2rgb function

#9940 added the test for black special case

https://www.vagrearg.org/content/hsvrgb << some optimization for that function (avoiding float)


Co-authored-by: Benjamin Valentin <benpicco@beuth-hochschule.de>
Co-authored-by: Karl Fessel <karl.fessel@ovgu.de>
@bors
Copy link
Contributor

bors bot commented Jun 5, 2023

Build failed (retrying...):

@maribu
Copy link
Member

maribu commented Jun 5, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 5, 2023

Already running a review

bors bot added a commit that referenced this pull request Jun 5, 2023
@bors @benpicco @kfessel
Merge #19378 #19693
1124998 
19378: sys: add common imath module mv isin() form test/driver_dac_dds r=maribu a=benpicco



19693: sys/color: extend unittest and fix module r=kfessel a=kfessel

### Contribution description

this extends the unittest for sys_color testing more colors 

### Testing procedure

```
RIOT_tree/tests/unittests$ make tests-color test
```
will fail since our `rgb2hsv` implementation is wrong (or is using an other colorspace than hsv2rgb (without documenting))

the new `hsv2rgb` test will succeed

### Issues/PRs references

#19614  was the reason i had a look at this 

#1315 added the rgb2hsv and hsv2rgb function

#9940 added the test for black special case

https://www.vagrearg.org/content/hsvrgb << some optimization for that function (avoiding float)


Co-authored-by: Benjamin Valentin <benpicco@beuth-hochschule.de>
Co-authored-by: Karl Fessel <karl.fessel@ovgu.de>
@bors
Copy link
Contributor

bors bot commented Jun 5, 2023

Build failed (retrying...):

bors bot added a commit that referenced this pull request Jun 5, 2023
@bors @kfessel
Merge #19693
f178daf 
19693: sys/color: extend unittest and fix module r=kfessel a=kfessel

### Contribution description

this extends the unittest for sys_color testing more colors 

### Testing procedure

```
RIOT_tree/tests/unittests$ make tests-color test
```
will fail since our `rgb2hsv` implementation is wrong (or is using an other colorspace than hsv2rgb (without documenting))

the new `hsv2rgb` test will succeed

### Issues/PRs references

#19614  was the reason i had a look at this 

#1315 added the rgb2hsv and hsv2rgb function

#9940 added the test for black special case

https://www.vagrearg.org/content/hsvrgb << some optimization for that function (avoiding float)


Co-authored-by: Karl Fessel <karl.fessel@ovgu.de>
@bors
Copy link
Contributor

bors bot commented Jun 5, 2023

Build failed:

@maribu
Copy link
Member

maribu commented Jun 6, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Jun 6, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 24a26c9 into RIOT-OS:master Jun 6, 2023
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maribu maribu maribu approved these changes

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@benpicco benpicco Awaiting requested review from benpicco

Assignees
No one assigned
Labels
Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
Release 2023.07
Development

Successfully merging this pull request may close these issues.
5 participants
@kfessel @miri64 @riot-ci @maribu @benpicco