Add support for LPC55S16.#30
Conversation
9e9aa14 to
24520ff
Compare
|
Isn't the whole point of chiptool to be able to fix bad SVD files? Can we not patch a fix ourselves using chiptool until then? |
3a05b55 to
d9f410a
Compare
|
This is based on my directory move PR. If we can get that one landed, I'm happy to land this one and iterate on fixes via transforms after the fact. |
|
I would prefer we iterate over the transforms here at least a little bit. It doesn't have to be final, but it still needs to go through review. etc. When you want the transforms reviewed, move the PR from draft to ready and add the reviewers. I am still unsure what you mean by
Please see my previous comment. |
|
The bug in chiptool accepts a bad description in the SVD. I have an direct example in the linked issue. I do think that chiptool needs to, at the very least, needs to report that such an issue is happening. I honestly think it should default to erroring out with maybe a flag to generate the code anyway. Assuming it's possible to fix this particular problem with only transforms, I didn't want to spend time to write the transforms to fix it since the fixed SVD is coming from NXP. This holding this ass a draft. With regard to transforms in general, there are a ton of things we could do to make the loc55s16 pac coffee seem more sane. Like there are a ton of enums with options with names like It also feels important to note that the enums have the same problems in LPC55S69, but only one is rewritten by the transforms for that board. That honestly seems worse than changing them all. |
766f4e8 to
7e8c1da
Compare
|
I've changed my mind. I marked this as ready for review so that it can hopefully be landed. I guess if we need to regenerate the code for a new version of the svd, we can do that in the future. Landing it will also make it easier for me to start working on nxp-hal changes. FWIW, I do think that transforms are a bit more reasonable to iterate on after landing this. I hope we can do that. Thanks. |
|
@eva-cosma I think I've addressed everything. please let me know if there is anything else. |
f52a095 to
b7c5dcb
Compare
|
I went ahead and used the transforms from the lpc55s69 after verifying them. I have a diff that is compiling for embassy-nxp that I have not yet published that is based on this change. |
|
I am considering another commit on this stack to move the transform |
|
The commits on this PR include #46. The transforms needed to be zhuzhed since there is an inconsistency between the SVDs for S16 and S69. |
eva-cosma
left a comment
There was a problem hiding this comment.
No. I don't want a common mega-file for all lpc55. I understand the rationale, but the core idea is that the LPC55s69 yaml file needs to be refactored to include comments and proper explanations as to what is happening and why we are doing in that file. Making a "common" file now, will just make it harder in the future to track what we did for the lpc55s69. Leave that one alone, for now. I would much much much rather have data duplicated in 2 places than spread amongst many files. More so that if someone makes a change in the common LPC file, it should be tested on all variations. Not easily feasible and the code is not "final" enough for that.
When someone implements a 3rd LPC chip we can think of a common file.
| - !MergeEnums | ||
| from: iocon::vals::Pio\d+Asw | ||
| to: iocon::vals::PioAsw |
There was a problem hiding this comment.
Because the SVDs different in the naming of a particular enum. On the S16, the enum values are named so that the enum is removed by the useless removal feature of chiptool. On the S69, that removal does not happen, and these rules are applied to rewrite the enum.
A better solution would be to make that single bit field resolve to a bool instead of the enum and remove the enum. Then the S16 and S69 would be identical in that regard.
| - !AddFields | ||
| fieldset: iocon::regs::Pio | ||
| fields: | ||
| - name: asw | ||
| description: "Analog switch input control." | ||
| bit_offset: 10 | ||
| bit_size: 1 | ||
| enum: iocon::vals::PioAsw | ||
|
|
| || x.starts_with("CARGO_FEATURE_LPC55S16") | ||
| || x.starts_with("CARGO_FEATURE_LPC55S69") |
There was a problem hiding this comment.
I think you can merge this into a singe CARGO_FEATURE_LPC55
|
Does this mean that there is no goal of trying to keep them as similar as possible? |
|
I don't want to touch the lpc55s69 transforms. Not yet. We can brainstorm later how to split it up and make stuff reusable. But I don't have the bandwidth to do that now, and I don't want to slow down this PR even more. |
|
Okay, so I reverted the changes with the S69 and copied the S69 transforms to the S16 ones except for the ones that needed to be removed since they are not relevant to the S16. |
No description provided.