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

zdsp: macros-based utilities #79457

Closed

Conversation

josuah
Copy link
Collaborator

@josuah josuah commented Oct 5, 2024

This adds a macro implementation of basic fixed-point operations.

The short-term goal was to implement this:

This could be interesting for implementing drivers that feature some light computation on scalars, for which a DSP library would be an overkill, and which might not be available in all platforms the driver covers.

This also helps with the current DSP libraries by being usable in const expressions, i.e. initialize tables of data:

#define COEF(x) SUBq7(Q7f(1.0), Q7f(x / 3.141592653589793))
const q7_t coefficients[] = { COEF(1.32), COEF(1.42), COEF(0.8), COEF(0.33), COEF(0.23) };

Or be intervleaved with the current ZDSP library

q15_t array[32];

[... ZDSP computations]

array[0] = Q15f(0.1);
array[31] = Q15f(0.1);

Unsolved questions:

  • Does it make sense to have it there? Or to implement it that way?
  • Is the naming suitable? i.e. Q15_ADD is more verbose.
  • Should rounding be implemented?
  • Should overflow be checked? I.e. on MAP_VALUE()

Todo:

  • Add tests for the fixed-point library.
  • Add documentation for the fixed-point library.

@josuah josuah added RFC Request For Comments: want input from the community DNM This PR should not be merged (Do Not Merge) labels Oct 5, 2024
@josuah josuah requested review from yperess and stephanosio October 5, 2024 16:41
@josuah josuah marked this pull request as draft October 5, 2024 16:42
@zephyrbot zephyrbot added area: DSP area: Base OS Base OS Library (lib/os) labels Oct 5, 2024
* @param omin The minimum value of the output range.
* @param omax The maximum value of the output range.
*/
#define MAP_VALUE(i, imin, imax, omin, omax) \
Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen Oct 5, 2024

Choose a reason for hiding this comment

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

MAP may be a non optimal fit here, since maps map any input to any output, possibly in both directions, and even mapping 1 to many values :)

0 -> 100
3 -> 67
5 -> 45
7 -> (3, 5)
...

while scaling "linearly" maps an input to an output, you even hint at this in the brief :)

#define SCALE_VALUE(...)

would be better fitting IMO.

Copy link
Collaborator Author

@josuah josuah Oct 5, 2024

Choose a reason for hiding this comment

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

Absolutely, I could not find the best word and picked the same as Arduino library.

I am tempted to use SCALE(): as out of the 5 pages of search results, it shew-up 5 times with that same meaning (1 2 3 4 5)

Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

Please add a unit test to validate the macros :)

@josuah
Copy link
Collaborator Author

josuah commented Oct 5, 2024

Since it is to be included right next to a CMSIS-based library, it makes sense that it gives the exact same results...
I could try an unoptimized (or rather only compiler-optimized) zdsp back-end using it.
This would allow testing and check that the same results are obtained by running the same tests suite.

I am still working on the <zephyr/dsp/macros.h> own unit tests.
Grateful for the feedback!

This permits to make a computation in a larger range to gain precision:

	int32_t tmp_in = SCALE(in, -100, 100, INT32_MIN, INT32_MAX);

Or convert between two unit systems:

	uint32_t fahrenheit = SCALE(celsius, 0, 100, 32, 212);

Signed-off-by: Josuah Demangeon <[email protected]>
@josuah josuah force-pushed the pr-zdsp-macros branch 2 times, most recently from ecdb2bb to 30b1107 Compare October 6, 2024 00:56
@josuah
Copy link
Collaborator Author

josuah commented Oct 6, 2024

Force-push:

  • Add full documentation
  • Add partial tests (WIP)

@josuah josuah force-pushed the pr-zdsp-macros branch 4 times, most recently from 6172d10 to 767d1ee Compare October 6, 2024 02:02
Saturation logic is supported, but not rounding.

This helps as a support for inputting literal values:

	q15_t first = Q15f(0.23);

This permits to store Q values in consts expressions:

	#define VAL(x) SUBq7(Q7f(1.0), Q7f(x / M_PI))
	const q7_t table[] = { VAL(1.32), VAL(1.42), VAL(0.8) };

This permits to use fixed point arithmetics when a zdsp back-end is
not available, such as light computation in drivers subitted upstream.

Signed-off-by: Josuah Demangeon <[email protected]>
@josuah
Copy link
Collaborator Author

josuah commented Oct 6, 2024

Force-push:

  • Tests for Q7 that pass
  • Doc fixes
  • Removing the % operator as hard to define "remainder" of a float division

Copy link

github-actions bot commented Dec 6, 2024

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Dec 6, 2024
@josuah
Copy link
Collaborator Author

josuah commented Dec 6, 2024

I still wish to build some C "fallback" back-end for ZDSP (i.e. for RISC-V) and other utilities, and would pick-up this PR again when having more time for it.

@josuah josuah closed this Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Base OS Base OS Library (lib/os) area: DSP DNM This PR should not be merged (Do Not Merge) RFC Request For Comments: want input from the community Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants