Skip to content
This repository was archived by the owner on Nov 11, 2025. It is now read-only.

Implement the KiCad expression evaluator (for real this time)#14

Merged
luxas merged 8 commits into
mainfrom
kicad-evaluator
Jun 21, 2021
Merged

Implement the KiCad expression evaluator (for real this time)#14
luxas merged 8 commits into
mainfrom
kicad-evaluator

Conversation

@twelho
Copy link
Copy Markdown
Member

@twelho twelho commented Jun 18, 2021

This took much longer than expected, but the KiCad evaluator is finally here, fully functional and ready to roll. What a Rust adventure and learning experience this has been 😅 This initial implementation of the evaluator supports the following features (out of all currently planned features):

  • Evaluation of all default and custom-defined expressions defined in component attributes.
  • Full namespace support, attributes of components defined in child schematics in the KiCad hierarchical schematic model can be accessed via <schematic>.<component>.<attribute>, e.g. DCDC.R3.Tolerance. (Parent schematics cannot be accessed by design to preserve the capability of evaluating every schematic independently.)
  • Order-independent evaluation using a recursive dependency resolver.
  • Self-dependency and circular dependency detection.
  • Automatic appending of the unit for an attribute if provided.
  • Automatic parsing of numeric types for the value field (if no string-based unit is given).
  • Support for global overrides (using text field parsing or a custom KiCad component).
  • Support for support functions such as voltage_divider(...) (either baked-in or loaded externally).
  • Applying the updates from the internal Schematic format back into the kicad_parse_gen::schematic::Schematic (Implement updating and exporting kicad_parse_gen Schematics for the evaluator #18).

As evidenced by the rather large commits, this has gone through many revisions until I finally figured out how to implement this properly in Rust. Thus, I suggest squashing the commits when merging. I'm also aware that the code is a bit lightly commented right now due to lack of time, but I will improve that front with upcoming PRs. I can however also add the comments to this PR if that's preferred.

cc @luxas @chiplet

@twelho twelho added the enhancement New feature or request label Jun 18, 2021
@twelho twelho requested review from chiplet and luxas June 18, 2021 22:01
@twelho twelho self-assigned this Jun 18, 2021
@twelho twelho force-pushed the kicad-evaluator branch from 3992894 to 1d8c596 Compare June 18, 2021 22:07
Copy link
Copy Markdown
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work with this one 💯 🥇

Comment thread tools/kicad_rs/src/bin/evaluator.rs
Comment thread tools/kicad_rs/src/eval.rs Outdated
// Collect all attributes for all components
let mut paths = Vec::new();
for (k, v) in index.map.iter() {
if let Node::Component(idx) = v {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use comp or something similar here? that'd be clearer

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose idx for index to specifically distinguish that what's stored there is a lookup table, not any sort of Component object. But I've spelt it (and some nearby fields as well) out now for better clarity.

Comment thread tools/kicad_rs/src/eval.rs Outdated
Comment thread tools/kicad_rs/src/eval/entry.rs
Comment on lines +12 to +14
pub struct SheetIndex<'a> {
pub(crate) map: HashMap<String, Node<'a>>,
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is SheetIndex a struct by ComponentIndex a type re-definition?
Is that the only way to add functions to the type (I guess so)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly right, in Rust you can only impl for types that are defined in the same crate. With a type alias I would be impling for HashMap, which wouldn't be possible since it is defined externally. ComponentIndex just didn't need any additional functionality other than the type safety it provides, so it's simpler to have it as an alias. For the SheetIndex I'm also using the pub(crate) visibility modifier for map to restrict external accessors accidentally modifying it.


pub fn resolve_entry<'b>(
&self,
mut path: impl ExactSizeIterator<Item = &'b String>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fancy, haven't seen this kind of impl statement before

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's essentially a simpler variant of a generic with a trait bound in Rust 2018 (1.26 and above). There are some restrictions around the turbofish operator with it compared to explicit generics, but in this simple case it makes no meaningful difference other than this syntax being likely a bit more readable.

@luxas
Copy link
Copy Markdown
Member

luxas commented Jun 21, 2021

Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants