Add tempmon abstruction #115

Alexander89 · 2022-01-04T19:07:42Z

I got a little bit side trapped by the Temperature Monitor (TEMPMON)

I thought it is barely simple and a great start in getting my hands dirty with the imxrt-ral ecosystem.

The interrupts are not really handy.

Open to discuss

I played around with the UX a little bit and finally, it is OK.

In my test app, I implemented a static alarm flag to note, when I have to enable the interrupts again.
Another method would be, to stop the measurement process, but as soon you read the temperature, the interrupt is triggered again.

// skip rest

let mut t = peripherals.tempmon.init_with_measure_freq(0x100);
t.set_alarm_values(40.0, 70.0, 85.0);
let (low_alarm, high_alarm, panic_alarm) = t.alarm_values();
log::debug!("low {}, high {}, panic {}", low_alarm, high_alarm, panic_alarm);

// v2: AtomicBool 
static mut ALARM: bool = false;
#[cortex_m_rt::interrupt]
fn TEMP_LOW_HIGH() {
  unsafe { ALARM = true; }
  TempMon::disable_interrupts(true, false);
  cortex_m::asm::dsb();
}

loop {
  let temp = t.get_temp();
  if temp > low_alarm && temp < high_alarm && unsafe { alarm } == true {
    unsafe { alarm = false; }
    t.enable_interrupts(true, true);
  }
}

If anybody has a great idea on how to improve it, please let me know 😃

teburd

Thank you for contributing!

I think in general the idea is really nice, and the API seems reasonable. A few things though..

The usage of floats in the API is, in my opinion, is less than ideal. It's ok if you have a few helper functions alongside this as const fn's if you like, but float as a whole tend to be troublesome. Yes this micro has a fpu so perhaps its less of an issue, but generally speaking, on a microcontroller and it's SDK you aren't likely going to see floats in use.

Does it make sense as well to make the powered state of this peripheral part of its type state to help the programmer ensure the peripheral is in the desired state at various code points where it might be used?

Consider someone calls get_temp(), or measure_temp on an unpowered tempmon, what happens if its off?

imxrt-hal/src/tempmon.rs

Alexander89 · 2022-01-04T23:33:46Z

Consider someone calls get_temp(), or measure_temp on an unpowered tempmon, what happens if it's off?

Actually, get_temp() still works or it just returns old data. So, somehow it doesn't really work, though.
But measure_temp() will just loop to the end of time.

I will add a power_on check and as you mention and I turn the values into a Result.

One additional question:

Should I change the messure_temp() into non-blocking and return WouldBlock in the blocking case. (nb::block!() compatible)

teburd · 2022-01-05T00:24:55Z

Should I change the messure_temp() into non-blocking and return WouldBlock in the blocking case. (nb::block!() compatible)

Seems reasonable, a Result return here makes sense as it is fallible if the device isn't on. Perhaps using nb here would be nice, I haven't used it in an async way myself though so I can't comment one way or the other here

Alexander89 · 2022-01-05T10:20:03Z

ich changed the math to °mC and adopted the docs to that.

In addition, I implemented an error handling to deal with the power_down state.

The temp_mon.measure_temp() is now compatible to nb::block!()

I also ran cargo clippy and cargo fmt, I hope the pipeline is now happy :-)

Alexander89 · 2022-01-05T10:23:04Z

@mciantyre, I created an example app to test this code, should I add it to teensy4-rs/examples?

mciantyre

Thanks for the new peripheral. A few questions and observations from my first review.

I created an example app to test this code, should I add it to teensy4-rs/examples?

Happy to maintain it there. Thanks!

imxrt-hal/src/tempmon.rs

imxrt-hal/src/lib.rs

imxrt-hal/src/tempmon.rs

mciantyre · 2022-01-06T01:10:03Z

imxrt-hal/src/tempmon.rs

+            ral::write_reg!(
+                ral::tempmon,
+                self.base,
+                TEMPSENSE0,
+                MEASURE_TEMP: 1
+            );


This write_reg! sets the MEASURE_TEMP bit high, and sets all other register bits to zero. We're similarly using write_reg! throughout this implementation to change one field in a register. Is this causing any issues in practice?

Would modify_reg, which preserves the unspecified fields, be more appropriate? Or, could we use the various *_SET, *_CLR, and *_TOGGLE registers with write_reg! to achieve our goals? It might be better to write this 1 to TEMPSENSE0_SET, since the hardware will preserve the other bits. (I'd recommend using the set/clear/toggle registers when possible. We could then discuss &mut self vs &self receivers for these methods.)

I changed it, please let me know if this is how it should be. I didn't find an example.

I think it should be &mut self.
Otherwise, somebody could change the behavior of the tempmon without having a mutable reference.

I changed it, please let me know if this is how it should be. I didn't find an example

Found it, modify_reg!() is what I was looking for

Were there issues using the *_SET and *_CLR register variants? I noticed a commit that added them, then another commit to prefer modify_reg!.

I just changed it, because I didn't really understand how it works. but using *_SET and *_CLR feels much faster!

Ahh I'm sorry I wasn't clear. The latest commit shows you're using these registers pefectly. Thanks for taking another look!

Quick summary for future PR readers:

All high bits written to a *_SET register will set the corresponding in the normal register

All high bits written to a *_CLR register will clear the corresponding bits in the normal register

All high bits written to a *_TOG register will toggle the corresponding bits in the normal register

Note that we're always writing high bits, even if the end result is to produce low bits in the register.

Section 1.5.2 of the 1060 reference manual provides more discussion about these special registers. In particular,

[...] it is more efficient to simply set or clear the target bit [...]. A simple set or clear operation is also atomic, while a CS [clear then set] operation is not.

I see your point about taking &mut self, since this operation changes the peripheral's state. But on the other hand, just as we can safely modify an atomic behind a shared reference &self, we might be able to safely start, stop, power up, and power down the tempmon peripheral behind a &self when using these special registers. (Let's not change anything in this PR, since I'm still looking for feedback on this idea.)

imxrt-hal/src/tempmon.rs

mciantyre · 2022-01-06T01:33:31Z

imxrt-hal/src/tempmon.rs

+                // this could be triggered again without any effect
+                Err(nb::Error::WouldBlock)
+            } else {
+                self.measurement_active = false;


Is it important to maintain this state in RAM? This seems like information we already have in the MEASURE_TEMP field. Instead of setting this bool to false, we could set MEASURE_TEMP = 0.

I thought about that as well, but there is an edge-case.

The Chip sets MEASURE_TEMP to 0 as soon he finished the measurement in single-shot mode.

If MEASURE_TEMP is 0 again, I would start the next measurement and never reach the TEMP_CNT section

I'm sorry, but I didn't find a better solution. This is really not a nice solution :-(

Thanks for clarifying. I read that FINISHED clears, but I didn't read that the hardware could also clear MEASURE_TEMP. If that't the case, this seems perfect.

believe it or not, I double-checked it and I didn't know why I assumed this behavior.

Probably I expected this that way because I start the next measurement with a 1.
I learned a lot about memory-mapping and how it works. Thanks for your patience :-)

No problem! Thanks for double checking, and for updating the approach. It wouldn't have surprised me if the behavior was undocumented / missing in the reference manual 😛.

teburd

I believe all the vital changes have been made, some wording could be improved in some comments such as describing parameters and returns, especially units involved when they aren't clear.

mciantyre · 2022-01-07T22:21:05Z

imxrt-hal/src/tempmon.rs

+                // this could be triggered again without any effect
+                Err(nb::Error::WouldBlock)
+            } else {
+                self.measurement_active = false;


Thanks for clarifying. I read that FINISHED clears, but I didn't read that the hardware could also clear MEASURE_TEMP. If that't the case, this seems perfect.

imxrt-hal/src/tempmon.rs

mciantyre · 2022-01-08T12:22:37Z

imxrt-hal/src/tempmon.rs

+            ral::write_reg!(
+                ral::tempmon,
+                self.base,
+                TEMPSENSE0,
+                MEASURE_TEMP: 1
+            );


Were there issues using the *_SET and *_CLR register variants? I noticed a commit that added them, then another commit to prefer modify_reg!.

imxrt-hal/src/tempmon.rs

Alexander89 · 2022-01-08T22:16:18Z

Thanks for all your patience! I learned a lot about the RAL layer and many other things.
🙏 🚀

I think this is it now 🎉

mciantyre

Looks awesome. Thank you for working through this!

OK with me to merge. Leaving this open in case @teburd wants any other documentation updates. Otherwise, I'll merge and forward port to the main branch sometime next / the following weekend.

We can use #116 to discuss how we want to release this feature.

mciantyre · 2022-01-09T16:52:38Z

imxrt-hal/src/tempmon.rs

+            ral::write_reg!(
+                ral::tempmon,
+                self.base,
+                TEMPSENSE0,
+                MEASURE_TEMP: 1
+            );


Ahh I'm sorry I wasn't clear. The latest commit shows you're using these registers pefectly. Thanks for taking another look!

Quick summary for future PR readers:

All high bits written to a *_SET register will set the corresponding in the normal register

All high bits written to a *_CLR register will clear the corresponding bits in the normal register

All high bits written to a *_TOG register will toggle the corresponding bits in the normal register

Note that we're always writing high bits, even if the end result is to produce low bits in the register.

Section 1.5.2 of the 1060 reference manual provides more discussion about these special registers. In particular,

[...] it is more efficient to simply set or clear the target bit [...]. A simple set or clear operation is also atomic, while a CS [clear then set] operation is not.

I see your point about taking &mut self, since this operation changes the peripheral's state. But on the other hand, just as we can safely modify an atomic behind a shared reference &self, we might be able to safely start, stop, power up, and power down the tempmon peripheral behind a &self when using these special registers. (Let's not change anything in this PR, since I'm still looking for feedback on this idea.)

mciantyre · 2022-01-09T17:01:20Z

imxrt-hal/src/tempmon.rs

+                // this could be triggered again without any effect
+                Err(nb::Error::WouldBlock)
+            } else {
+                self.measurement_active = false;


No problem! Thanks for double checking, and for updating the approach. It wouldn't have surprised me if the behavior was undocumented / missing in the reference manual 😛.

imxrt-hal/src/tempmon.rs

teburd · 2022-01-09T20:42:26Z

Noted the places where I saw units for parameters and returns might be useful, I believe its mC -> C and vice versa, but would be nice to have it noted there.

Alexander89 added 2 commits January 4, 2022 19:06

add tempmon

24d3dab

add tempmon support

779a87c

teburd reviewed Jan 4, 2022

View reviewed changes

imxrt-hal/src/tempmon.rs Outdated Show resolved Hide resolved

Alexander89 added 2 commits January 5, 2022 11:08

changing to mC to avoid f32

b755195

fixing documentation

89c17ff

Alexander89 requested a review from teburd January 5, 2022 10:24

figuring out that ```ignore fixed the pipeline

9b8c96c

mciantyre reviewed Jan 6, 2022

View reviewed changes

review fixes

d1f7ab8

Alexander89 requested a review from mciantyre January 6, 2022 18:05

use modify_reg

1239b04

teburd approved these changes Jan 7, 2022

View reviewed changes

mciantyre reviewed Jan 8, 2022

View reviewed changes

mciantyre mentioned this pull request Jan 8, 2022

0.4 HAL releases may break RAL users #116

Open

review

94793b8

mciantyre approved these changes Jan 9, 2022

View reviewed changes

teburd approved these changes Jan 9, 2022

View reviewed changes

imxrt-hal/src/tempmon.rs Show resolved Hide resolved

imxrt-hal/src/tempmon.rs Outdated Show resolved Hide resolved

impr. docs

2f92588

teburd merged commit 2ab80a7 into imxrt-rs:maint-v0.4 Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tempmon abstruction #115

Add tempmon abstruction #115

Alexander89 commented Jan 4, 2022

teburd left a comment •

edited

Loading

Alexander89 commented Jan 4, 2022

teburd commented Jan 5, 2022

Alexander89 commented Jan 5, 2022

Alexander89 commented Jan 5, 2022

mciantyre left a comment

mciantyre Jan 6, 2022

Alexander89 Jan 6, 2022

Alexander89 Jan 6, 2022

mciantyre Jan 8, 2022

Alexander89 Jan 8, 2022

mciantyre Jan 9, 2022

mciantyre Jan 6, 2022

Alexander89 Jan 6, 2022

mciantyre Jan 7, 2022

Alexander89 Jan 8, 2022

mciantyre Jan 9, 2022

teburd left a comment

mciantyre Jan 7, 2022

mciantyre Jan 8, 2022

Alexander89 commented Jan 8, 2022

mciantyre left a comment

mciantyre Jan 9, 2022

mciantyre Jan 9, 2022

teburd commented Jan 9, 2022

Add tempmon abstruction #115

Add tempmon abstruction #115

Conversation

Alexander89 commented Jan 4, 2022

Open to discuss

teburd left a comment • edited Loading

Choose a reason for hiding this comment

Alexander89 commented Jan 4, 2022

One additional question:

teburd commented Jan 5, 2022

Alexander89 commented Jan 5, 2022

Alexander89 commented Jan 5, 2022

mciantyre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexander89 commented Jan 8, 2022

mciantyre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd commented Jan 9, 2022

teburd left a comment •

edited

Loading