Skip to content

weather: Fix incorrect rounding for negative temperatures #2194

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

Closed
FintasticMan wants to merge 2 commits into from
Closed

weather: Fix incorrect rounding for negative temperatures #2194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c868a31
weather: Fix incorrect rounding for negative temperatures
FintasticMan Dec 9, 2024
635aa0c
utility: Add function for calculating a rounded division
FintasticMan Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/components/ble/SimpleWeatherService.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#undef min

#include "components/datetime/DateTimeController.h"
#include "utility/Math.h"

int WeatherCallback(uint16_t connHandle, uint16_t attrHandle, struct ble_gatt_access_ctxt* ctxt, void* arg);

Expand Down Expand Up @@ -75,11 +76,11 @@ namespace Pinetime {
}

[[nodiscard]] int16_t Celsius() const {
return (PreciseCelsius() + 50) / 100;
return Utility::RoundedDiv(PreciseCelsius(), 100);
}

[[nodiscard]] int16_t Fahrenheit() const {
return (PreciseFahrenheit() + 50) / 100;
return Utility::RoundedDiv(PreciseFahrenheit(), 100);
}

bool operator==(const Temperature& other) const {
Expand Down
5 changes: 5 additions & 0 deletions src/utility/Math.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
#pragma once

#include <cstdint>
#include <concepts>

namespace Pinetime {
namespace Utility {
// returns the arcsin of `arg`. asin(-32767) = -90, asin(32767) = 90
int16_t Asin(int16_t arg);

static constexpr auto RoundedDiv(std::integral auto dividend, std::integral auto divisor) -> decltype(dividend / divisor) {
Copy link
Member

@mark9064 mark9064 Nov 4, 2025

Choose a reason for hiding this comment

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

I think we should note the overflow properties in a docstring. abs(dividend) + abs(divisor) / 2 must be less than the type maximum I think? Also I think it would be useful to note whether both the dividend and divisor can be negative

Copy link
Contributor

@NeroBurner NeroBurner Nov 4, 2025

Choose a reason for hiding this comment

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

I think adding a docstring/comment what we expect from this function would be helpful. And as @mark9064 said what are the constraints on the inputs (if any)

Copy link

@Andersama Andersama Nov 30, 2025

Choose a reason for hiding this comment

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

I'm all for ? operators but where things are strictly math based...might as well write using strictly math.

int16_t fixed_rounding(int16_t value, int16_t divisor) {
	int16_t signed_value = ((uint16_t)value >> 14u) & 0x2; //if signed 2 otherwise 0
	int16_t half_divisor = (divisor / 2);
    //add a half, subtract divisor conditionally on sign
	return (value + half_divisor - half_divisor*signed_value) / divisor;
}
//same as above except we don't calculate divisor*
int16_t fixed_rounding_b(int16_t value, int16_t divisor) {
	int16_t signed_value = (uint16_t)value >> 15u; //if signed 1 otherwise 0
	int16_t half_divisor = (divisor / 2);
	return (value + half_divisor - divisor * signed_value) / divisor;
}

Copy link

@Andersama Andersama Nov 30, 2025

Choose a reason for hiding this comment

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

Or...if you trust bithacky madness, no * needed.

int16_t fixed_rounding(int16_t value, int16_t divisor) {
	int16_t signed_value = value >> 15u; // true evil: we use >>'s signed behavior to propagate the leading 1 across all bits, eg: we have 0xffff or 0x0000
	int16_t half_divisor = (divisor / 2);
	return (value + half_divisor - (divisor & signed_value)) / divisor; //we replace the * by "1" with an &
}

return (dividend + (dividend >= 0 ? divisor : -divisor) / 2) / divisor;
}
}
}