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

The functions set_using_xosc(), set_using_lposc(), set_using_gpio_1khz(), set_using_gpio_1hz() in \src\rp235x\powman\regs.rs do not make sense #3789

Closed
mschnell1 opened this issue Jan 21, 2025 · 6 comments

Comments

@mschnell1
Copy link

mschnell1 commented Jan 21, 2025

The functions set_using_xosc(), set_using_lposc(), set_using_gpio_1khz(), set_using_gpio_1hz() in \src\rp235x\powman\regs.rs do not make sense
as the appropriate bits in the TIMER register are read only.

@mschnell1
Copy link
Author

Moreover IMHO, the function set_alarm(&mut self, val: bool) should be called reset_alarm() and have no argument, as the appropriate bit in the TIMER register is "Clear on write 1".

@mschnell1
Copy link
Author

mschnell1 commented Jan 27, 2025

BTW.: it would be nice if the "Register" functions would support the non consuming Builder Pattern .
such as:

TIMER.time()
  .read()
  .set_nonsec_write()
  .clear()
  .set_alarm_enab(false)
  .reset_alarm()
  .set_key()
  .write(TIMER.time())

With set_key() setting the upper 16 bits to 0x5afe

Seems like a non breaking change to me.

@Dirbaio
Copy link
Member

Dirbaio commented Jan 27, 2025

the PAC is autogenerated by chiptool. It's not possible to do ad-hoc changes to individual registers like that, they all have to work the same way.

It's intentional that all fields are readable/writable. The "register value" structs are a copy of the register in memory. Think of it like a "u32 but with a nice wrapper to get/set all the fields".

Chiptool intentionally doesn't support "chaining" method calls like that because it results in less readable code.

@mschnell1
Copy link
Author

mschnell1 commented Jan 27, 2025

Yep (it took me some trial and error but) I do understand the "nice wrapper" concept and it's perfectly OK.
I don't agree and think chaining and not seeing "dummy" and always "appropriately named" functions would be even nicer, but I do see your meaning.
(In fact my suggestion would need chiptool to read a configuration file with alternate conversions: maybe not actually viable)
Thanks for answering !

BTW.: Embassy is just GREAT !
BTW/2 Seemingly I am close to succeeding in doing a timer_driver version that allows to select (by features) if you want to use TIMER1,TIMER2, or the zero Power AON TIMER as the Embassy system timer.

@mschnell1
Copy link
Author

mschnell1 commented Jan 28, 2025

Ahh, chiptool is part of the embasy-rp repository !
So I might come up with an issue suggesting enhancement over there....

@mschnell1
Copy link
Author

mschnell1 commented Jan 28, 2025

I found that the Builder Pattern does not make as much sense, as I supposed, because the write function's target needs to be repeated, even if it's a constant: .write(TIMER.time()) .

It could be memorized from .read(), but that would impose some ambiguity.

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

No branches or pull requests

2 participants