-
Notifications
You must be signed in to change notification settings - Fork 45
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
[LARGE] add the ability to parse matter idl files and start the ability to code generate based on them #120
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Aside from two nits regarding typos, my other comments are in the area of:
- How to arrange the IDL parsing+codegen code in such a way so that it is as less confusing for the user as possible (naming, number of crates etc.)
- Whether we are on the same page w.r.t. expectations from the IDL parser/generator - e.g. everything in the existing
on_off
module except theHandler
impl should be possible to auto-generate from IDL defs - including structures for commands and command responses by the way - Last, but not least - how do we imagine the IDL generator to be used
If you can give a few examples of inconsistencies. Not taking offence with that statement at all - just trying to understand where you see those. Also, for me, the IDL generation does not solve a consistency problem, but rather - the annoying problem that unless we have the IDL generation, users (or us) will have to write tons of boilerplate code (what I call the clusters' metadata definitions) for all the clusters which are defined in the Matter Cluster Library spec. Are we on the same page here? |
Hi, my complaint about consistency was that when I was comparing cluster_media_playback.rs, cluster_on_off.rs and cluster_basic_information.rs the content was ever so slightly different so I could not find the one format to generate. I started from the top and immediately hit differences in attribute declarations like:
Final goal is to not have humans type out repetitive code and more easily import data from the cpp repo or spec one. we also get some more consistent behaviour as a sideffect (and requirement). We can tweak that if we really need things to differ, however I imagine we do not. |
|
My personal take on this PR is that - once @andreilitvin confirms that the set of changes we negotiated so far are ready - we should merge it. The PR is unobtrusive to the existing code-base, and once merged, the IDL generator can be further improved, extended and - possibly - even utilized for our own I'm personally excited of the prospects it opens. |
@ivmarkov I agree, I think we can merge this and iterate on the merged entity faster, it is a step in the right direction. @andy31415 I like the idea of moving the existing stuff in I have some questions on the
Not suggesting we address these within this PR itself. We can go ahead and merge if @andreilitvin you are at a stable enough point. |
To some extent this might be alleviated if we expose "user-friendly" cluster-specific handler traits which are adaptable to the generic Consider this code that the macro might be generating: pub trait OnOffHandler {
// Attributes
async fn get_on_off(&self) -> Result<bool, Error>;
async fn get_global_scene_control(&self) -> Result<bool, Error>;
// Commands
async fn off(&self) -> Result<(), Error>;
async fn on(&self) -> Result<(), Error>;
async fn toggle(&self) -> Result<(), Error>;
async fn off_with_effect(&self, off_with_effect_request: &OffWithEffectRequest<'_>) -> Result<(), Error>;
async fn on_with_recall_global_scene(&self) -> Result<(), Error>;
async fn on_with_timed_off(&self) -> Result<(), Error>;
}
// Adapts `OnOffHandler` to the generic `AsyncHandler` trait
pub struct AsyncHandlerImpl<T>(T);
impl<T> AsyncHanlderImpl<T> {
pub const fn new(handler: T) -> Self {
Self(handler)
}
}
impl<T> AsyncHandler for AsyncHandlerImpl<T>
where
T: OnOffHandler,
{
async fn read(&self, ...) {
todo!() // Call the relevant get_XXX attribute method on T
}
async fn write(&self, ...) {
todo!() // Call the relevant set_XXX attribute method on T
}
async fn invoke(&self, ...) {
todo!() // Call the relevant command
}
} In a way, the Furthermore, we would still generate e.g. the Combined with auto-generated documentation (say, we can put a comment // IDL: readonly attribute optional boolean globalSceneControl = 16384 on top of the auto-generated |
I could not find a good way to simply The generated code will be somewhat large: every structure (including even/request/response) will be generated and every attribute/command will have a separate item too. At first I imagine cargo-expand can be used to view what gets generated and unit tests also show generated code. I think we have to decide where our source of truth is - this PR proposes .idl because those can be copied over from the CPP sdk and then it is guaranteed to match the sdk view of the spec. Later we could use data model XML which are based on spec text. We are not precluded from using the parser without macros and generating code and checking that in - I think if that becomes more approachable in the future we can go with that to.
I think we can extract what the macro would generate manually (cargo expand or similar), hand edit until we are happy and then put it back in the macro. I am unsure about the best flow, however iteration for now is reasonably friendly as the macro takes about 10ms.
I think it is as stable as we should for a first merge. It is a 8K |
To be clear, I wasn't proposing we go the Going ahead with the merge now. |
The current data models in data_model are not very consistent. Looking to have a single code generation that makes it consistent.
For now I added:
Integration is done via rs-matter-macros, however I wonder if the
rs-matter-macros
should be generally exported somewhere else to allow for unit testing (most code should be in a non-proc-macro2 module to allow tests).IDL parser code coverage should be decent, however I have no experience in data model development for clusters, so the macro bits and integration is still lacking. I re-export some bits that I should probably not.
Feedback would be appreciated.