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

Toggle-only access policy support? #14

Open
stephenwhittle opened this issue Jul 6, 2019 · 5 comments
Open

Toggle-only access policy support? #14

stephenwhittle opened this issue Jul 6, 2019 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@stephenwhittle
Copy link

I'm working on refactoring a small USB stack for STM32 to avoid its dependencies (CMSIS, HAL etc) and have my own svd->cppreg header generation flow which is working great.
However, the STM32 USB registers, specifically the endpoint registers, have a number of fields which are toggle-only (ie read is permitted, write 1 to flip) mixed in with normal read-write fields.

Would it be feasible to devise an access policy which changes the way cppreg attempts to maintain the value of a field?

Writing the existing value back, as I presume the existing implementation would do (to try to preserve the value of the toggle fields) when I'm trying to write to a normal field in the same register, will cause all the toggles to flip if they are currently set.

Happy to entertain other suggestions, too of course. CMSIS handles this just fine by forcing all toggle field values to 0 during read-modify-write, but to do the same I'd have to be able to read the entire packed register at once, then do a merge_write to all fields, and packed registers don't support read().
Looking over the documentation it seems that perhaps a modification of the shadow register functionality could achieve this, too?

@sendyne-nicocvn sendyne-nicocvn added the enhancement New feature or request label Jul 7, 2019
@sendyne-nicocvn
Copy link
Contributor

Hi @stephenwhittle
First, I am glad to hear that cppreg is working fine for you.

I first want to confirm what this issue is about. Say we have a register of the form:

.... Field A Field B ....
.... 1 0 ....

Assuming Field A and Field B are "write 1 to toggle" the issue is that if you
write 1 to Field B the current cppreg implementation will "rewrite" the value 1
in Field A thus leading to:

.... Field A Field B ....
.... 0 1 ....

while you would want to have:

.... Field A Field B ....
.... 1 1 ....

Can you confirm that my understanding is correct? I can think of several ways to
tackle this issue but I first want to make sure I understand correctly.

As a side note, it is possible to read the register memory (for both packed and
normal registers). Register memory handling in the current version of cppreg
(v0.4) was significantly revised compared to previous versions: registers and
packed registers now go through the same facility to manager the memory (see the
Memory header). This means that if you have a type Reg (packed or not)
you can use:

  • Reg::rw_mem_device() to get a read-write access to the memory,
  • Reg::ro_mem_device() to get a read-only access to the memory.

Note that similar functions are also available in previous cppreg versions but
the details of their implementations differ.

@sendyne-nicocvn sendyne-nicocvn self-assigned this Jul 7, 2019
@stephenwhittle
Copy link
Author

Yes, you're correct in your understanding of the issue at hand. Thanks for the reminder re cppreg versions too - I'm using it as a submodule and had somehow forgotten to update. It might be worth mentioning rw_mem_device etc in the quick start guide, if you have a few moments to include a note on it, as I wasn't aware of that at all.
With that in mind, it looks like a call to ro_mem_device to get the existing state, followed by a merge_write call to all the fields in the register, would give me the equivalent behaviour to the original implementation, but I'm happy to hear your alternative suggestions.

@sendyne-nicocvn
Copy link
Contributor

Ok I created #15 and documentation will be updated very soon.

Regarding this issue a few comments first:

  • the shadow value implementation is not really designed for this type of issue; it is intended to be used for write-only registers and I do not see a clear path on how to implement handling of "toggle fields",
  • we could implement a dedicated access policy for such fields but this does not solve the problem; what would be needed is for the register to be aware that it contains "toggle fields" so that it could properly manage the read/write for these fields.

Your approach (reading the whole register and managing the writes through merge_write) is typically what we do in various edge cases that requires additional logic for group of fields. If you want I can add to cppreg a generic implementation of a "managed register" that could handle toggle fields or other edge cases. This is in my opinion the best approach.

The other solution is to add more logic to the register implementation with custom access policies but this will be less generic than the "managed register" approach and will take more time as I would have to think on how to do it in practice.

@stephenwhittle
Copy link
Author

The managed register idea sounds like the best way to accomplish this in an extensible way, so that gets my vote. I was just really dreading having to manage this single register CMSIS-style to avoid messing up the toggles :)

@sendyne-nicocvn
Copy link
Contributor

Sounds good; will add the manager register implementation as soon as possible.

@nicocvn nicocvn assigned nicocvn and unassigned sendyne-nicocvn Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants