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

Create a function library for the evaluator, bring back the ICCC voltage divider #16

Merged
merged 4 commits into from
Jun 24, 2021

Conversation

twelho
Copy link
Member

@twelho twelho commented Jun 23, 2021

This moves kicad_rs and the new function library kicad_functions into a common Cargo workspace to facilitate dependency version sharing as well as make running stuff like cargo fmt for the project easier.

The new voltage divider function is based on https://github.com/trebinor/resistor-calc, which I had to fork and modify a bit to get to compile without its own expression evaluator. PR tracking upstreaming here: trebinor/resistor-calc#4.

Support list for voltage_divider:

Continuation of #14, ticking one more box in the support list there. The last commit 9a9a43f is just test data for evaluating the voltage divider and may be removed before merging. That test data configuration is identical to what was passed to ICCC for the BD9E DC-DC converter, and this Rust code does output the same optimal resistance values, so it should be working correctly.

cc @luxas @chiplet

@twelho twelho added the enhancement New feature or request label Jun 23, 2021
@twelho twelho requested review from luxas and chiplet June 23, 2021 17:54
Copy link
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.

Nicely done! The resistor-calc crate is great 😄

@@ -0,0 +1,11 @@
[package]
name = "kicad_functions"
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason to make this a new crate compared to build into kicad_rs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly just for easy expandability, one does not need to touch core application logic to extend its functionality (via adding these functions). IIRC the "external library" approach was also preferred when I asked about it.

@@ -270,6 +270,8 @@ F 0 "C2" H 3592 4046 50 0000 L CNN
F 1 "10u" H 3592 3955 50 0000 L CNN
F 2 "Capacitor_SMD:C_1206_3216Metric" H 3500 4000 50 0001 C CNN
F 3 "~" H 3500 4000 50 0001 C CNN
F 4 "" H 3500 4000 50 0001 C CNN "DividerTest"
F 5 "vdiv(5.1, \"(R1+R2)/R2*0.8\", \"E96\", 500e3, 700e3)" H 3500 4000 50 0001 C CNN "DividerTest_expr"
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit unclear on how to use this correctly, is it a map you index like vdiv(...)[2] to get the R2 voltage?

Copy link
Member Author

Choose a reason for hiding this comment

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

The function returns a tuple, it's described in the docs of the voltage_divider function. You need to store the result to a variable (like DividerTest here, but preferrably stored globally) to avoid running the (potentially very long) computation over and over again. What I have indeed not tested is indexing the evalexpr::Value(TupleType), but I sure hope that evalexpr supports indexing its own tuples.

if let Some(res) = calculate(&config) {
// Take the first result, these are ordered by increasing error
if let Some((v, set)) = res.iter().next() {
let voltage = config.target + *v as f64 / 1e9;
Copy link
Member

Choose a reason for hiding this comment

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

why this 1e9 constant?

Copy link
Member Author

@twelho twelho Jun 24, 2021

Choose a reason for hiding this comment

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

Taken straight from resistor-calc, it computes using fixed-point numbers (u64) that need to be converted to floats explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add parentheses to signal in what order the computation happens and a comment why this is like this (e.g. a link to another place this constant is used)? Or better, make this a dedicated helper

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a dedicated helper with an explanation now.

fn parse(v: &Value) -> EvalexprResult<Self> {
let tuple = v.as_tuple()?;

let resistance_min = tuple.get(3).map(|v| v.as_number()).transpose()?;
Copy link
Member

Choose a reason for hiding this comment

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

TIL about transpose, nice

@@ -360,3 +362,41 @@ impl<T: AsRef<str>> SplitCharN for Option<T> {
}
}
}

fn unescape(s: &str) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

Is this a "custom" unescaping function, or is it a "conventional" like done for e.g. URL escaping?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its custom, I just cobbled it together since kicad_parse_gen messes up escaping so badly.

for i in 1..=config.count {
tuple.push(Value::from(set.r(i)));
}
return Ok(Value::from(tuple));
Copy link
Member

Choose a reason for hiding this comment

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

Does evalexpr support tuples natively, or are they arrays?

Copy link
Member Author

Choose a reason for hiding this comment

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

The "tuple" is just a Vec with any amount of elements. evalexpr has a an enum variant Value(TupleType), where TupleType is an alias for Vec<Value>. I haven't checked what is supported for those though, i.e. indexing is pretty much required for this use case. Will check in a bit if that's implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for the info

@twelho
Copy link
Member Author

twelho commented Jun 24, 2021

After a bit of investigation it doesn't seem like there's built-in support for indexing tuples. I'll create an indexing function, hold on...

@twelho twelho force-pushed the evaluator-functions branch from 9a9a43f to 909923a Compare June 24, 2021 11:49
@twelho twelho force-pushed the evaluator-functions branch 2 times, most recently from c65bd24 to aeb3f3a Compare June 24, 2021 13:32
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

Successfully merging this pull request may close these issues.

2 participants