Skip to content

drivers: sensor: tenstorrent: pvt: refactor driver#877

Draft
alexapostolu wants to merge 2 commits intotenstorrent:mainfrom
alexapostolu:temp
Draft

drivers: sensor: tenstorrent: pvt: refactor driver#877
alexapostolu wants to merge 2 commits intotenstorrent:mainfrom
alexapostolu:temp

Conversation

@alexapostolu
Copy link
Member

@alexapostolu alexapostolu commented Dec 2, 2025

Refactor the PVT driver. There's been some CI failures where temperature readings have been outside the [40, 70] range.

The readings beyond 70 are no more than 71, so we increase the max to 75.

The readings below 40 hover near 0. I can't reproduce it, but I suspect this might be a data conversion issue. I refactored the PVT driver to have less data conversions, to use q31_t instead of float, and just basic refactoring to make the code simpler.

@alexapostolu
Copy link
Member Author

@afongTT Just confirming, is 75 a reasonable max limit for the temperature test? There's at least two CI runs now that have the chip temperature at 71 C

@cfriedt
Copy link
Contributor

cfriedt commented Dec 4, 2025

I've seen a bunch of tests where it's failing at close to zero degrees (at least according to logs), so well outside of the [40,70] range.

In this case, it's on yyz-zephyr-02 so is in the Toronto 11th floor lab. To me, it does not make any sense why we would be reading close to 0 degrees C.

https://github.com/tenstorrent/tt-zephyr-platforms/actions/runs/19913215921/job/57086440153

@alexapostolu
Copy link
Member Author

@cfriedt I'll take another look at the PVT driver to see if there's a bug in the implementation

@cfriedt
Copy link
Contributor

cfriedt commented Dec 4, 2025

@alexapostolu - are we is not floating point in Pvt?

@alexapostolu
Copy link
Member Author

alexapostolu commented Dec 4, 2025

@cfriedt We're not using the float type directly but we are still representing the decimal place using struct sensor_value.

@afongTT
Copy link
Contributor

afongTT commented Dec 4, 2025

@afongTT Just confirming, is 75 a reasonable max limit for the temperature test? There's at least two CI runs now that have the chip temperature at 71 C

Yeah, I think increasing the max to 75 or 80 would be reasonable. But the 0 degree reading isn't right.

@alexapostolu alexapostolu force-pushed the temp branch 3 times, most recently from e6b12d3 to fa56728 Compare December 9, 2025 16:15
@alexapostolu alexapostolu marked this pull request as ready for review December 9, 2025 16:15
@alexapostolu alexapostolu marked this pull request as draft December 9, 2025 16:17
@alexapostolu alexapostolu changed the title app: smc: pytest: e2e_smoke.py: increase temp max to 75 drivers: sensor: tenstorrent: pvt: refactor driver Dec 15, 2025
@alexapostolu alexapostolu force-pushed the temp branch 2 times, most recently from 4a6dc23 to 8606b69 Compare December 17, 2025 19:54
@alexapostolu alexapostolu marked this pull request as ready for review December 17, 2025 20:53
@alexapostolu alexapostolu marked this pull request as draft January 7, 2026 15:59
@alexapostolu
Copy link
Member Author

alexapostolu commented Jan 15, 2026

I don't think this PR can be merged anytime soon. To merge it, then we either have to add functions just for converting between raw sensor value to telemetry value and vice versa (which would be redundant), or change the arc message format (with won't be until the next major version upgrade).

Increase maximum temperature allowed in tests to 75 celcius. This is
because a failure has been observed on at least two runs where the
temperature reached 71 celcius. So increase it to 75 to be safe, and 75
is still a reasonable temperature for the chip.

Signed-off-by: Alex Apostolu <aapostolu@tenstorrent.com>
* Basic refactoring
* Remove streaming implementation
* Use raw value instead of the deprecated sensor_value
* Remove sensor_value conversion functions

Signed-off-by: Alex Apostolu <aapostolu@tenstorrent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants