[WIP] Adding encode functions to SVD elements

[ryankurte]

Work in progress, adding encode functions to SVD elements to allow exporting of optimised / modified SVD files. Also probably worth neatening up some of the parse/encode functions.

This is a step towards svd2rust#96.

@japaric do you have any thoughts / are you happy with an approach like this?

[pftbest]

making _extensible field public kinda defeats the purpose.

[ryankurte]

Hey, thanks for the review!

Ahh, yeah. I couldn't work out how to initialise an object with private fields / didn't find anything with wild googling.

The best solution I came up with was to create new constructors for each object, is that the best way to keep it hidden?

[pftbest]

Sorry, i don't know the best solution here.
When I was writing my msp430_svd tool I've made a local copy of this whole repo and just removed the private fields from all the elements. But I did it because I was too lazy to write a proper constructor for each struct.

btw, thank you for working on this, i'd be happy to go back to the upstream library when it gets serialization support.

[ryankurte]

Running out of steam, but, almost there...

A few of notes.

I have split out and implemented encoding + tests for pretty much everything now, there are rather a lot of changes :-/

This could (in future) be lossless so that encode == decode, for now the encoding is opinionated based on the file I was looking at at the time. This also might be nicer because all our processed files would then follow the same output standard.

I haven't yet gone through and written default constructors to fix the _extensible problem.

There's a repeated custom try! macro through the code that I don't really understand enough to remove, but it would be nice to either use the builtin or rename and properly include in the package.

Types now all implement PartialEq so that test assertions work, this is probably going to break share-svd. I guess we could implement Equivalent and Subset/Superset traits for future work there, but also open to any ideas and this probably should go in japaric/svd2rust#96.

[homunkulus]

☔ The latest upstream changes (presumably cd5aefa) made this pull request unmergeable. Please resolve the merge conflicts.

[ryankurte]

That's uhh, more than a small merge conflict :-/

I have removed and re-added the changes because it's so heavily diverged, and it almost builds again except for an equality (and uh, recursion?) issue I am not sure about. Either, Register, and Cluster all have derived PartialEq as far as I can tell.

error[E0369]: binary operation `==` cannot be applied to type `std::vec::Vec<either::Either<register::Register, cluster::Cluster>>`
  --> src/clusterinfo.rs:32:5
   |
32 |     pub children: Vec<Either<Register, Cluster>>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: an implementation of `std::cmp::PartialEq` might be missing for `std::vec::Vec<either::Either<register::Register, cluster::Cluster>>`

error[E0369]: binary operation `!=` cannot be applied to type `std::vec::Vec<either::Either<register::Register, cluster::Cluster>>`
  --> src/clusterinfo.rs:32:5
   |
32 |     pub children: Vec<Either<Register, Cluster>>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: an implementation of `std::cmp::PartialEq` might be missing for `std::vec::Vec<either::Either<register::Register, cluster::Cluster>>`

error: aborting due to 2 previous errors

error: Could not compile `svd-parser`.

It also has no tests / doesn't encode the new types. Would you be willing to merge a partial WIP once it's building again (even into a develop branch or something)? I guess it's deserved for opening such a big PR, but I /really/ don't want to have to do anything like this again.

[japaric]

@ryankurte thanks for working on this. I've been wanting to rewrite / refactor this crate for a while since it's kind of a mess and it's lacking proper error handling.

Regarding _extensible

If we are going to provide encoding functionality we must also provide constructors for the SVD structs. The current version doesn't provide any constructor: _extensible is there to both prevent constructing SVD structs and let us add fields without breaking the API.

If we are going to add constructors then I think the constructor should only take as arguments the mandatory fields and initialize all the optional fields to None. Something like this:

pub struct EnumeratedValue {
    pub name: String,
    pub value: Option<u32>,
    pub description: Option<String>,
    _extensible: (),
}

let ev = EnumeratedValue::new("foo"); // name = "foo", value = None, ..
ev.value = Some(1);

We could provide a builder like interface to sugar things up:

let e: Element = EnumeratedValue::new("foo").value(1).description("bar").encode();

Regarding try!

That try! is my half-assed attempt at "making it easier to add proper error handling later on". Back when I wrote this I didn't know about error-chain which what I think we should use (See next part)

Proper error handling and diagnostics

Quite a few people have commented that "svd2rust crashes without explanation" and "svd2rust doesn't provide any error message when malformed SVD files". All the problems they are seeing stem from the panicky SVD parser svd2rust uses: this crate.

To fix that we have to add proper error handling to this parser. I have made a proof of concept SVD parser that uses error-chain to report not too useless error messages with best effort span information (xmltree gives us zero span information).

Example error messages are shown below:

$ cargo run --example parse -- examples/bad-1.xml
<enumeratedValue>
    <name></name>
</enumeratedValue>

+ error: parsing `<enumeratedValue>`
| parsing `<name>`
| no inner text

$ cargo run --example parse -- examples/bad-2.xml
<enumeratedValue>
</enumeratedValue>

+ error: parsing `<enumeratedValue>`
| missing obligatory `<name>` element

$ cargo run --example parse -- examples/bad-3.xml
<enumeratedValue>
    <name>foo</name>
    <foo></foo>
</enumeratedValue>

+ error: parsing `<enumeratedValue>`
| named `foo`
| found unexpected `<foo>` element (child #1)

$ cargo run --example parse -- examples/bad-4.xml
<enumeratedValue>
    <name>foo</name>
    <value>true</value>
</enumeratedValue>

+ error: parsing `<enumeratedValue>`
| named `foo`
| parsing `<value>` (child #1)
| `true` is not a `u32 `

Thoughts about this PR

I like the new layout. It makes sense to have each SVD struct into its own module to keep them focused. The tests are also a great addition.

I'm not sure what the Uint struct, with these encoding and width fields, is buying the user since they extra info is hidden. I assume this hidden information is used to make the encode_decode tests work?. Oh, actually now I see that they are not being used ... yet?

Ideally I'd like to see the next minor release have proper error handling but I understand it's a lot of work, more than the encoding stuff being added here, and I don't think I'll have time to implement that any time soon.

So, hmm, I think we can land this if (a) it doesn't break the existing API and (b) it doesn't extend the API in odd ways, like _extensible becoming public. I think pub(crate) _extensible may help there: in theory it should be make the field public for the crate but not to external crates though I have never used it myself.

Also glob imports are kind of discouraged (with the exception of prelude imports) so they should be replaced with normal imports.

And extern crate xmltree is not required in each file since it's already in the root.

[ryankurte]

Proper error handling is definitely a need, i'm not totally sure I follow your example, but bubbling errors up seems like it shouldn't be super difficult / mostly requires the addition of result to the parse / encode types.

I implemented the parse/encode traits with an eye to the possibility of moving it over to a serde custom serialisation, which would take care of a bunch of the error handling stuff and reduce the amount of code I think. There is an XML implementation, but it's missing a bunch of the features we need (like attributes) atm.

Both are things I would be happy to try / work on down the track, but, I really don't want to keep growing this PR. That last merge back from master was a bit ghastly 😂
As an alternative to putting it all here I would be happy with working against a development branch if you wanted to create one / document it's existence so future PRs are made against it? Otherwise landing it and continuing would work for me.

I ripped try! out into a separate file for now so it's not duplicated everywhere, presumably we can drop it when we push error handling through.

Constructors / builders seem reasonable, I can add that / it should clean up some of the internal logic too I think.

The Uint struct is an experiment relating to a decision that needs to be made about whether encoding / decoding is lossless or not. If we want parse/encode to output the same file, we need to keep track of the underlying value formats in the xml files.
If we don't worry about that / are happy with specifying a standard for encoding/widths of different fields it can be removed.
I'm in the second camp in that I don't think we should worry about trying to output exactly the same file so long as the data is the same, which means we will end up with consistently formatted outputs. That said, wouldn't be against ripping the formatting stuff out to allow it to be configurable down the track, but again, not in this PR.

I was trying not to / don't think I have broken the API, but, given the previous one had no tests I can't really say that I didn't :-/
I guess compiling rust2svd and share-svd against this version would be a good test?

So my TODOs to get this merged are:

• Hide _extensibles (quick fix with pub(crate) _extensible)
• Clean up glob imports
• Remove extern references from subcomponents
• Fix missing PartialEq for std::vec::Vec<either::Either<register::Register, cluster::Cluster>> so we're back to compiling
• Finish adding encoding / tests to fields introduced in last merge
• Test an upgrade to svd2rust

Anything missing from that list?

[ryankurte]

Added a test and tried to rebuild svd2rust but it's broken with the latest version because of the PR that was conflicting with this :-/

[homunkulus]

☔ The latest upstream changes (presumably #42) made this pull request unmergeable. Please resolve the merge conflicts.

[ryankurte]

Bump.

The things implemented here are:

• Refactor to separate components for each type from lib.rs (would still love to see this happen, but, as a major change it's practically incompatible with any open PRs)
• Added encode methods for each type (so we can get working on the pipeline from #96
• Added encode-decode tests for each type (because otherwise I have no confidence the encode methods were correct)

This PR is probably way too out of date to rebase with all the changes that've made it in ahead, but, if we still want components of this I can rework them and open separate PRs?

My proposal would be that each component gets a PR that:

1. Moves it into it's own source file
2. Adds encode method
3. Adds encode/decode tests

That way any collisions will only be with one source file at a time, and refactoring around it should be less of a nightmare.

Any thoughts @japaric @pftbest @Emilgardis?

[Emilgardis]

I'll think this is a good idea. I do however suggest that we in between 1 and 2 (or maybe even before) squeeze in error handling. I have a branch where I'm testing this.

[ryankurte]

Seems sensible, though we could just apply your error handling branch prior to doing this work?
If we're going to put error handling in as part of each PR I guess we'll need to put in the base error stuff along with base encode/decode traits first anyhow.
Shall I create a new issue to document the refactor (and shall we do it all in a refactor branch perhaps)?

[ryankurte]

Closed in favour of #46 and #53.