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

sys/phydat: Fix unit confusion #19292

Merged
merged 1 commit into from
Feb 23, 2023
Merged

sys/phydat: Fix unit confusion #19292

merged 1 commit into from
Feb 23, 2023

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 21, 2023

Contribution description

Previously, UNIT_G was used for g-force with the correct symbol g, UNIT_GR for gram (as in kilogram) with the incorrect symbol G (which would be correct for Gauss), and UNIT_GS for Gauss with symbol Gs (which is an alternative correct symbol).

To avoid confusion between G-Force, Gauss, and Gram the units have been renamed to UNIT_G_FORCE, UNIT_GRAM, and UNIT_GAUSS. In addition, gram now uses the correct symbol g; which sadly is the same as for g-force. But usually there is enough context to tell them apart.

Testing procedure

Green CI

Issues/PRs references

None

@maribu maribu added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 21, 2023
@maribu maribu requested a review from bergzand February 21, 2023 11:24
@maribu maribu requested a review from miri64 as a code owner February 21, 2023 11:24
@github-actions github-actions bot added Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework labels Feb 21, 2023
@riot-ci
Copy link

riot-ci commented Feb 21, 2023

Murdock results

✔️ PASSED

3c287c0 sys/phydat: Fix unit confusion

Success Failures Total Runtime
6865 0 6865 10m:40s

Artifacts

@benpicco
Copy link
Contributor

doc in senml/phydat.h needs an update

@benpicco
Copy link
Contributor

tests/phydat_dump is the next one

Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

I see only one needing rework:

https://en.wikipedia.org/wiki/Standard_gravity

ɡ0 or gn

why rename gs to gaus?

@benpicco
Copy link
Contributor

IMO GAUSS and G_FORCE are much more readable than GS and GN

@maribu
Copy link
Member Author

maribu commented Feb 21, 2023

ɡ0 or gn

Standard gravity and gravitational force equivalent is not the same thing. Using that would confuse more than help.

why rename gs to gaus ?

Because the symbol for Gauss is G. So it should have been UNIT_G. But honestly, using the symbol in the macro name was a stupid idea to begin with. Is UNIT_A ampere, or is it Angstrom, or is it year (annus)? If we would have used UNIT_AMPERE, UNIT_ANGSTROM, and UNIT_YEAR instead, there would be no confusion.

@maribu
Copy link
Member Author

maribu commented Feb 21, 2023

bors merge

sys/phydat/phydat_str.c Outdated Show resolved Hide resolved
Previously, `UNIT_G` was used for g-force with the correct symbol `g`,
`UNIT_GR` for gram (as in kilogram) with the incorrect symbol `G` (which
would be correct for Gauss), and `UNIT_GS` for Gauss with symbol `Gs`
(which is an uncommon but correct symbol).

To avoid confusion between G-Force, Gauss, and Gram the units have been
renamed to `UNIT_G_FORCE`, `UNIT_GRAM`, and `UNIT_GAUSS`. In addition,
gram now uses the correct symbol `g` and Gauss uses `G`.
@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 23, 2023

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@benpicco
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 23, 2023

Stopped waiting for PR status (GitHub check) without running due to duplicate requests to run. You may check Bors to see that this PR is included in a batch by one of the other requests.

bors bot added a commit that referenced this pull request Feb 23, 2023
19292: sys/phydat: Fix unit confusion r=benpicco a=maribu

### Contribution description

Previously, `UNIT_G` was used for g-force with the correct symbol `g`, `UNIT_GR` for gram (as in kilogram) with the incorrect symbol `G` (which would be correct for Gauss), and `UNIT_GS` for Gauss with symbol `Gs` (which is an alternative correct symbol).

To avoid confusion between G-Force, Gauss, and Gram the units have been renamed to `UNIT_G_FORCE`, `UNIT_GRAM`, and `UNIT_GAUSS`. In addition, gram now uses the correct symbol `g`; which sadly is the same as for g-force. But usually there is enough context to tell them apart.

### Testing procedure

Green CI

### Issues/PRs references

None

19307: nanocoap_link_format: fix off-by-one error r=miri64 a=benpicco



Co-authored-by: Marian Buschsieweke <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
@miri64
Copy link
Member

miri64 commented Feb 23, 2023

bors cancel
bors merge

@bors
Copy link
Contributor

bors bot commented Feb 23, 2023

Canceled.

bors bot added a commit that referenced this pull request Feb 23, 2023
19292: sys/phydat: Fix unit confusion r=miri64 a=maribu

### Contribution description

Previously, `UNIT_G` was used for g-force with the correct symbol `g`, `UNIT_GR` for gram (as in kilogram) with the incorrect symbol `G` (which would be correct for Gauss), and `UNIT_GS` for Gauss with symbol `Gs` (which is an alternative correct symbol).

To avoid confusion between G-Force, Gauss, and Gram the units have been renamed to `UNIT_G_FORCE`, `UNIT_GRAM`, and `UNIT_GAUSS`. In addition, gram now uses the correct symbol `g`; which sadly is the same as for g-force. But usually there is enough context to tell them apart.

### Testing procedure

Green CI

### Issues/PRs references

None

19307: nanocoap_link_format: fix off-by-one error r=miri64 a=benpicco



Co-authored-by: Marian Buschsieweke <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
@miri64
Copy link
Member

miri64 commented Feb 23, 2023

bors cancel

@bors
Copy link
Contributor

bors bot commented Feb 23, 2023

Canceled.

@miri64
Copy link
Member

miri64 commented Feb 23, 2023

bors merge

bors bot added a commit that referenced this pull request Feb 23, 2023
19292: sys/phydat: Fix unit confusion r=miri64 a=maribu

### Contribution description

Previously, `UNIT_G` was used for g-force with the correct symbol `g`, `UNIT_GR` for gram (as in kilogram) with the incorrect symbol `G` (which would be correct for Gauss), and `UNIT_GS` for Gauss with symbol `Gs` (which is an alternative correct symbol).

To avoid confusion between G-Force, Gauss, and Gram the units have been renamed to `UNIT_G_FORCE`, `UNIT_GRAM`, and `UNIT_GAUSS`. In addition, gram now uses the correct symbol `g`; which sadly is the same as for g-force. But usually there is enough context to tell them apart.

### Testing procedure

Green CI

### Issues/PRs references

None

19307: nanocoap_link_format: fix off-by-one error r=miri64 a=benpicco



Co-authored-by: Marian Buschsieweke <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 23, 2023

This PR was included in a batch that was canceled, it will be automatically retried

@miri64
Copy link
Member

miri64 commented Feb 23, 2023

bors cancel
bors merge

@bors
Copy link
Contributor

bors bot commented Feb 23, 2023

Canceled.

@bors
Copy link
Contributor

bors bot commented Feb 23, 2023

Build succeeded:

@bors bors bot merged commit 4809b68 into RIOT-OS:master Feb 23, 2023
@maribu maribu deleted the sys/phydat branch February 24, 2023 05:36
@maribu
Copy link
Member Author

maribu commented Feb 24, 2023

Thx :)

bors bot added a commit that referenced this pull request Apr 25, 2023
19495: Rust: Update dependencies r=chrysn a=chrysn

### Contribution description

This updates both the RIOT-specific and generic dependencies of Rust examples and modules.

It also follows a deprecation from the G unit renaming originally done in #19292.

### Testing procedure

* Green CI should do

### Issues/PRs references

Copying from one of the commits with some sed:

* riot-sys:
    * RIOT-OS/rust-riot-sys#26
    * RIOT-OS/rust-riot-sys#31
    * RIOT-OS/rust-riot-sys#30
    * RIOT-OS/rust-riot-sys#28
    * RIOT-OS/rust-riot-sys#27
    * RIOT-OS/rust-riot-sys#25
    * RIOT-OS/rust-riot-sys#23
    * RIOT-OS/rust-riot-sys#22
    * RIOT-OS/rust-riot-sys#21
* riot-wrappers:
    * RIOT-OS/rust-riot-wrappers#36
    * RIOT-OS/rust-riot-wrappers#50
    * RIOT-OS/rust-riot-wrappers#48
    * RIOT-OS/rust-riot-wrappers#47
    * RIOT-OS/rust-riot-wrappers#44
    * RIOT-OS/rust-riot-wrappers#45
    * RIOT-OS/rust-riot-wrappers#43
    * (later, when the mistake became apparent) RIOT-OS/rust-riot-wrappers#55

### How to do similar PRs

Updating the RIOT-related dependencies (which here also updated bindgen because the dependency of riot-sys changed):

```
$ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml --package riot-sys --package riot-wrappers; done
```

Updating everything (should never really be needed, b/c if something has a concrete dependency change it'd say so, but dependencies generally get better over time):

```
$ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml; done
```

Creating the commit message:

```
$ git log --first-parent --format='%s (%b)' 9c29faf55d4c14d2d7f55f8df5059c52af4e5317..e4973a6ee88427f702dac41b3dce4fd6b6b9689e | sed 's/Merges: //' | sed 's/^/    * /'
```

git shortlog unfortunately doesn't show the merges the way I prefer linking them. The versions in the command line can be taken from `git diff --text` and looking for the riot-sys or riot-wrappers line, respectively.

Co-authored-by: chrysn <[email protected]>
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
bors bot added a commit that referenced this pull request Apr 28, 2023
19505: Rust: Update dependencies [backport 2023.04] r=maribu a=maribu

# Backport of #19495

### Contribution description

This updates both the RIOT-specific and generic dependencies of Rust examples and modules.

It also follows a deprecation from the G unit renaming originally done in #19292.

### Testing procedure

* Green CI should do

### Issues/PRs references

Copying from one of the commits with some sed:

* riot-sys:
    * RIOT-OS/rust-riot-sys#26
    * RIOT-OS/rust-riot-sys#31
    * RIOT-OS/rust-riot-sys#30
    * RIOT-OS/rust-riot-sys#28
    * RIOT-OS/rust-riot-sys#27
    * RIOT-OS/rust-riot-sys#25
    * RIOT-OS/rust-riot-sys#23
    * RIOT-OS/rust-riot-sys#22
    * RIOT-OS/rust-riot-sys#21
* riot-wrappers:
    * RIOT-OS/rust-riot-wrappers#36
    * RIOT-OS/rust-riot-wrappers#50
    * RIOT-OS/rust-riot-wrappers#48
    * RIOT-OS/rust-riot-wrappers#47
    * RIOT-OS/rust-riot-wrappers#44
    * RIOT-OS/rust-riot-wrappers#45
    * RIOT-OS/rust-riot-wrappers#43
    * (later, when the mistake became apparent) RIOT-OS/rust-riot-wrappers#55

### How to do similar PRs

Updating the RIOT-related dependencies (which here also updated bindgen because the dependency of riot-sys changed):

```
$ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml --package riot-sys --package riot-wrappers; done
```

Updating everything (should never really be needed, b/c if something has a concrete dependency change it'd say so, but dependencies generally get better over time):

```
$ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml; done
```

Creating the commit message:

```
$ git log --first-parent --format='%s (%b)' 9c29faf55d4c14d2d7f55f8df5059c52af4e5317..e4973a6ee88427f702dac41b3dce4fd6b6b9689e | sed 's/Merges: //' | sed 's/^/    * /'
```

git shortlog unfortunately doesn't show the merges the way I prefer linking them. The versions in the command line can be taken from `git diff --text` and looking for the riot-sys or riot-wrappers line, respectively.

Co-authored-by: chrysn <[email protected]>
bors bot added a commit that referenced this pull request Apr 28, 2023
19505: Rust: Update dependencies [backport 2023.04] r=maribu a=maribu

# Backport of #19495

### Contribution description

This updates both the RIOT-specific and generic dependencies of Rust examples and modules.

It also follows a deprecation from the G unit renaming originally done in #19292.

### Testing procedure

* Green CI should do

### Issues/PRs references

Copying from one of the commits with some sed:

* riot-sys:
    * RIOT-OS/rust-riot-sys#26
    * RIOT-OS/rust-riot-sys#31
    * RIOT-OS/rust-riot-sys#30
    * RIOT-OS/rust-riot-sys#28
    * RIOT-OS/rust-riot-sys#27
    * RIOT-OS/rust-riot-sys#25
    * RIOT-OS/rust-riot-sys#23
    * RIOT-OS/rust-riot-sys#22
    * RIOT-OS/rust-riot-sys#21
* riot-wrappers:
    * RIOT-OS/rust-riot-wrappers#36
    * RIOT-OS/rust-riot-wrappers#50
    * RIOT-OS/rust-riot-wrappers#48
    * RIOT-OS/rust-riot-wrappers#47
    * RIOT-OS/rust-riot-wrappers#44
    * RIOT-OS/rust-riot-wrappers#45
    * RIOT-OS/rust-riot-wrappers#43
    * (later, when the mistake became apparent) RIOT-OS/rust-riot-wrappers#55

### How to do similar PRs

Updating the RIOT-related dependencies (which here also updated bindgen because the dependency of riot-sys changed):

```
$ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml --package riot-sys --package riot-wrappers; done
```

Updating everything (should never really be needed, b/c if something has a concrete dependency change it'd say so, but dependencies generally get better over time):

```
$ for x in **/Cargo.lock; do cargo update --manifest-path=${x%.lock}.toml; done
```

Creating the commit message:

```
$ git log --first-parent --format='%s (%b)' 9c29faf55d4c14d2d7f55f8df5059c52af4e5317..e4973a6ee88427f702dac41b3dce4fd6b6b9689e | sed 's/Merges: //' | sed 's/^/    * /'
```

git shortlog unfortunately doesn't show the merges the way I prefer linking them. The versions in the command line can be taken from `git diff --text` and looking for the riot-sys or riot-wrappers line, respectively.

19509: cpu/cc26xx_cc13xx: Fix bogus array-bound warning [backport 2023.04] r=maribu a=maribu

# Backport of #19504

### Contribution description

GCC 12 create a bogus array out of bounds warning as it assumes that because there is special handling for `uart == 0` and `uart == 1`, `uart` can indeed be `1`. There is an `assert(uart < UART_NUMOF)` above that would blow up prior to any out of bounds access.

In any case, optimizing out the special handling of `uart == 1` for when `UART_NUMOF == 1` likely improves the generated code and fixes the warning.

    /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:88:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds]
       88 |     ctx[uart].rx_cb = rx_cb;
          |     ~~~^~~~~~
    /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx'
       52 | static uart_isr_ctx_t ctx[UART_NUMOF];
          |                       ^~~
    /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:89:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds]
       89 |     ctx[uart].arg = arg;
          |     ~~~^~~~~~
    /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx'
       52 | static uart_isr_ctx_t ctx[UART_NUMOF];
          |                       ^~~

### Testing procedure

The actual change is a pretty obvious one-liner, so that code review and a green CI should be sufficient. If not, running any UART example app without regression should do.

### Issues/PRs references

None

19510: tools/openocd: Fix handling of OPENOCD_CMD_RESET_HALT [backport 2023.04] r=maribu a=maribu

# Backport of #19506

### Contribution description

The OPENOCD_CMD_RESET_HALT was not longer correctly passed to the script. This fixes the issue.

### Testing procedure

Flashing of e.g. the `cc2650-launchpad` with upstream OpenOCD should work again.

### Issues/PRs references

The change was added to #19050 after testing the PR and before merging. I'm not sure if the fix never worked because of this, or if behavior of `target-export-variables` or GNU Make changed.

Co-authored-by: chrysn <[email protected]>
Co-authored-by: Marian Buschsieweke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework CI: no fast fail don't abort PR build after first error CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants