search

rust-osdev / acpi

Rust library for parsing ACPI tables and interpreting AML
Apache License 2.0
203 stars 65 forks source link

add methods to fetch raw data #194

Closed devsnek closed 1 year ago

devsnek commented 1 year ago

I found it necessary to be able to fetch data from the tables without monomorphizing in order to integrate with lai.

I also ran cargo fmt after I made my changes and a bunch of other stuff got picked up. Lemme know if I should remove those changes.

IsaacWoods commented 1 year ago

Hm, on the formatting I've run cargo fmt locally with an up-to-date rustfmt and I'm not getting any of the use formatting changes from this PR - maybe a config from a parent directory is affecting local behaviour for you or something? I don't think your formatting is standard, or fits this project's rustfmt settings.

On the new functionality, I'm not against the ability to use acpi with other AML parsers, but this doesn't feel like it fits our API that well so I'd like to understand your requirements a bit better. Can you share the code that glues this into the LAI interface?

I get the intention to do a straight port of their functions, but the signature of this function feels strange against our existing API. In particular, the name find_sdt is very similar to existing methods, but returns an AmlTable (where the majority of tables can't be returned as such). The requirement to manage multiple of each table is also particular to SSDTs (if I'm not mistaken?) and so that functionality can pretty easily be accomplished already with something like tables.ssdts().nth(index).unwrap(). Overall, I don't feel this is a worthwhile addition to the stable API for the confusion it's likely to create imo.

Let me know what you think.

IsaacWoods commented 1 year ago

We haven't heard from you in a while @devsnek, so I'll close this PR. Please re-open it if you'd like to continue working on this.

devsnek commented 1 year ago

@IsaacWoods sorry i didn't see this. lai needs to be provided a function which looks up a table address given its signature. the signature is just some random string, so it can't be generic over Signature like the existing lookup function. Happy to adjust this however you feel would be best.