| Message ID | 20210325134231.1400051-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote: > Asynchronous tasks can only be submitted while the IPA is running. , as they are queued to the IPA thread for execution and will thus never run if the IPA is stopped. > Further more, the shutdown sequence can not be tracked with a simple s/Further more/Furthermore/ ? > running flag. We can also be in the state 'Stopping' where we have not > yet completed all events, but we must not commence anything new. > > Refactor the running_ boolean into a stateful enum to track this. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++---- > .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++- > .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++-- Oh, you've now overcome the fear of touching the mojo templates. Be careful, you'll soon be considered a reference person in this area :-D > 3 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > index e3b541db4e36..dd0f4d3f64b6 100644 > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > @@ -45,7 +45,7 @@ namespace {{ns}} { > {%- endif %} > > {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) > - : IPAProxy(ipam), running_(false), > + : IPAProxy(ipam), state_(Stopped), > isolate_(isolate), seq_(0) > { > LOG(IPAProxy, Debug) > @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > > return {{ "_ret" if method|method_return_value != "void" }}; > {%- elif method.mojom_name == "start" %} > - running_ = true; > + state_ = Running; > thread_.start(); > > {{ "return " if method|method_return_value != "void" -}} > @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > {%- endfor -%} > ); > {% elif method|is_async %} > - ASSERT(running_); > + ASSERT(state_ == Running); > proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued, > {%- for param in method|method_param_names -%} > {{param}}{{- ", " if not loop.last}} > @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > {% for method in interface_event.methods %} > {{proxy_funcs.func_sig(proxy_name, method, "Thread")}} > { > - ASSERT(running_); > + ASSERT(state_ != Stopped); > {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}}); > } > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > index efb09a180b90..e33c26492d91 100644 > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > @@ -104,7 +104,13 @@ private: > {{interface_name}} *ipa_; > }; > > - bool running_; > + enum {{proxy_name}}States { > + Stopped, > + Stopping, > + Running, Maybe ProxyStopped, ProxyStopping, ProxyRunning to protect about namespace clashes ? > + }; Blank line ? > + enum {{proxy_name}}States state_; You can drop enum here. > + But I'd drop this one :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Thread thread_; > ThreadProxy proxy_; > std::unique_ptr<{{interface_name}}> ipa_; > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > index 8addc2fad0a8..09394a4fec83 100644 > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > @@ -23,9 +23,12 @@ > # \brief Generate function body for IPA stop() function for thread > #} > {%- macro stop_thread_body() -%} > - if (!running_) > + ASSERT(state_ != Stopping); > + if (state_ != Running) > return; > > + state_ = Stopping; > + > proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking); > > thread_.exit(); > @@ -33,7 +36,7 @@ > > Thread::current()->dispatchMessages(Message::Type::InvokeMessage); > > - running_ = false; > + state_ = Stopped; > {%- endmacro -%} > >
Hi Kieran, On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote: > Asynchronous tasks can only be submitted while the IPA is running. > > Further more, the shutdown sequence can not be tracked with a simple > running flag. We can also be in the state 'Stopping' where we have not > yet completed all events, but we must not commence anything new. > > Refactor the running_ boolean into a stateful enum to track this. Ooh, good idea! > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++---- > .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++- > .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++-- > 3 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > index e3b541db4e36..dd0f4d3f64b6 100644 > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > @@ -45,7 +45,7 @@ namespace {{ns}} { > {%- endif %} > > {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) > - : IPAProxy(ipam), running_(false), > + : IPAProxy(ipam), state_(Stopped), > isolate_(isolate), seq_(0) > { > LOG(IPAProxy, Debug) > @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > > return {{ "_ret" if method|method_return_value != "void" }}; > {%- elif method.mojom_name == "start" %} > - running_ = true; > + state_ = Running; > thread_.start(); > > {{ "return " if method|method_return_value != "void" -}} > @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > {%- endfor -%} > ); > {% elif method|is_async %} > - ASSERT(running_); > + ASSERT(state_ == Running); > proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued, > {%- for param in method|method_param_names -%} > {{param}}{{- ", " if not loop.last}} > @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > {% for method in interface_event.methods %} > {{proxy_funcs.func_sig(proxy_name, method, "Thread")}} > { > - ASSERT(running_); > + ASSERT(state_ != Stopped); > {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}}); > } As mentioned on irc, I think we need to copy these over to the IPC versions too, but that can be done on top. > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > index efb09a180b90..e33c26492d91 100644 > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > @@ -104,7 +104,13 @@ private: > {{interface_name}} *ipa_; > }; > > - bool running_; > + enum {{proxy_name}}States { > + Stopped, > + Stopping, > + Running, > + }; > + enum {{proxy_name}}States state_; > + Since these are the same for all IPAProxies, should we put them in ipa_proxy.h in the base IPAProxy class instead? With or without the suggested change, Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > Thread thread_; > ThreadProxy proxy_; > std::unique_ptr<{{interface_name}}> ipa_; > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > index 8addc2fad0a8..09394a4fec83 100644 > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > @@ -23,9 +23,12 @@ > # \brief Generate function body for IPA stop() function for thread > #} > {%- macro stop_thread_body() -%} > - if (!running_) > + ASSERT(state_ != Stopping); > + if (state_ != Running) > return; > > + state_ = Stopping; > + > proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking); > > thread_.exit(); > @@ -33,7 +36,7 @@ > > Thread::current()->dispatchMessages(Message::Type::InvokeMessage); > > - running_ = false; > + state_ = Stopped; > {%- endmacro -%} > > > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote: > Hi Kieran, > > On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote: > > Asynchronous tasks can only be submitted while the IPA is running. > > > > Further more, the shutdown sequence can not be tracked with a simple > > running flag. We can also be in the state 'Stopping' where we have not > > yet completed all events, but we must not commence anything new. > > > > Refactor the running_ boolean into a stateful enum to track this. > > Ooh, good idea! > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++---- > > .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++- > > .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++-- > > 3 files changed, 16 insertions(+), 7 deletions(-) > > > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > > index e3b541db4e36..dd0f4d3f64b6 100644 > > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > > @@ -45,7 +45,7 @@ namespace {{ns}} { > > {%- endif %} > > > > {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) > > - : IPAProxy(ipam), running_(false), > > + : IPAProxy(ipam), state_(Stopped), > > isolate_(isolate), seq_(0) > > { > > LOG(IPAProxy, Debug) > > @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > > > > return {{ "_ret" if method|method_return_value != "void" }}; > > {%- elif method.mojom_name == "start" %} > > - running_ = true; > > + state_ = Running; > > thread_.start(); > > > > {{ "return " if method|method_return_value != "void" -}} > > @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > > {%- endfor -%} > > ); > > {% elif method|is_async %} > > - ASSERT(running_); > > + ASSERT(state_ == Running); > > proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued, > > {%- for param in method|method_param_names -%} > > {{param}}{{- ", " if not loop.last}} > > @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > > {% for method in interface_event.methods %} > > {{proxy_funcs.func_sig(proxy_name, method, "Thread")}} > > { > > - ASSERT(running_); > > + ASSERT(state_ != Stopped); > > {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}}); > > } > > As mentioned on irc, I think we need to copy these over to the IPC > versions too, but that can be done on top. > > > > diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > > index efb09a180b90..e33c26492d91 100644 > > --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > > +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > > @@ -104,7 +104,13 @@ private: > > {{interface_name}} *ipa_; > > }; > > > > - bool running_; > > + enum {{proxy_name}}States { > > + Stopped, > > + Stopping, > > + Running, > > + }; > > + enum {{proxy_name}}States state_; > > + > > Since these are the same for all IPAProxies, should we put them in > ipa_proxy.h in the base IPAProxy class instead? I like this. > With or without the suggested change, > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > Thread thread_; > > ThreadProxy proxy_; > > std::unique_ptr<{{interface_name}}> ipa_; > > diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > > index 8addc2fad0a8..09394a4fec83 100644 > > --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > > +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > > @@ -23,9 +23,12 @@ > > # \brief Generate function body for IPA stop() function for thread > > #} > > {%- macro stop_thread_body() -%} > > - if (!running_) > > + ASSERT(state_ != Stopping); > > + if (state_ != Running) > > return; > > > > + state_ = Stopping; > > + > > proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking); > > > > thread_.exit(); > > @@ -33,7 +36,7 @@ > > > > Thread::current()->dispatchMessages(Message::Type::InvokeMessage); > > > > - running_ = false; > > + state_ = Stopped; > > {%- endmacro -%} > > > >
Hi Laurent / Paul, On 26/03/2021 02:52, Laurent Pinchart wrote: > On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote: >> Hi Kieran, >> >> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote: >>> Asynchronous tasks can only be submitted while the IPA is running. >>> >>> Further more, the shutdown sequence can not be tracked with a simple >>> running flag. We can also be in the state 'Stopping' where we have not >>> yet completed all events, but we must not commence anything new. >>> >>> Refactor the running_ boolean into a stateful enum to track this. >> >> Ooh, good idea!>> >>> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++---- >>> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++- >>> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++-- >>> 3 files changed, 16 insertions(+), 7 deletions(-) >>> >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >>> index e3b541db4e36..dd0f4d3f64b6 100644 >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >>> @@ -45,7 +45,7 @@ namespace {{ns}} { >>> {%- endif %} >>> >>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) >>> - : IPAProxy(ipam), running_(false), >>> + : IPAProxy(ipam), state_(Stopped), >>> isolate_(isolate), seq_(0) >>> { >>> LOG(IPAProxy, Debug) >>> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) >>> >>> return {{ "_ret" if method|method_return_value != "void" }}; >>> {%- elif method.mojom_name == "start" %} >>> - running_ = true; >>> + state_ = Running; >>> thread_.start(); >>> >>> {{ "return " if method|method_return_value != "void" -}} >>> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) >>> {%- endfor -%} >>> ); >>> {% elif method|is_async %} >>> - ASSERT(running_); >>> + ASSERT(state_ == Running); >>> proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued, >>> {%- for param in method|method_param_names -%} >>> {{param}}{{- ", " if not loop.last}} >>> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) >>> {% for method in interface_event.methods %} >>> {{proxy_funcs.func_sig(proxy_name, method, "Thread")}} >>> { >>> - ASSERT(running_); >>> + ASSERT(state_ != Stopped); >>> {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}}); >>> } >> >> As mentioned on irc, I think we need to copy these over to the IPC >> versions too, but that can be done on top. I'm not sure I understand this here ... >>> >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl >>> index efb09a180b90..e33c26492d91 100644 >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl >>> @@ -104,7 +104,13 @@ private: >>> {{interface_name}} *ipa_; >>> }; >>> >>> - bool running_; >>> + enum {{proxy_name}}States { >>> + Stopped, >>> + Stopping, >>> + Running, >>> + }; >>> + enum {{proxy_name}}States state_; >>> + >> >> Since these are the same for all IPAProxies, should we put them in >> ipa_proxy.h in the base IPAProxy class instead? > > I like this. I started with that and then moved it here for some reason, that I can no longer recall. But keeping it common is better I agree, so I'll move it back. > >> With or without the suggested change, >> >> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> >> >>> Thread thread_; >>> ThreadProxy proxy_; >>> std::unique_ptr<{{interface_name}}> ipa_; >>> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl >>> index 8addc2fad0a8..09394a4fec83 100644 >>> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl >>> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl >>> @@ -23,9 +23,12 @@ >>> # \brief Generate function body for IPA stop() function for thread >>> #} >>> {%- macro stop_thread_body() -%} >>> - if (!running_) >>> + ASSERT(state_ != Stopping); >>> + if (state_ != Running) >>> return; >>> >>> + state_ = Stopping; >>> + >>> proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking); >>> >>> thread_.exit(); >>> @@ -33,7 +36,7 @@ >>> >>> Thread::current()->dispatchMessages(Message::Type::InvokeMessage); >>> >>> - running_ = false; >>> + state_ = Stopped; >>> {%- endmacro -%} >>> >>> >
Hi Kieran, On Fri, Mar 26, 2021 at 12:27:46PM +0000, Kieran Bingham wrote: > On 26/03/2021 02:52, Laurent Pinchart wrote: > > On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote: > >> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote: > >>> Asynchronous tasks can only be submitted while the IPA is running. > >>> > >>> Further more, the shutdown sequence can not be tracked with a simple > >>> running flag. We can also be in the state 'Stopping' where we have not > >>> yet completed all events, but we must not commence anything new. > >>> > >>> Refactor the running_ boolean into a stateful enum to track this. > >> > >> Ooh, good idea!>> > >>> > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++---- > >>> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++- > >>> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++-- > >>> 3 files changed, 16 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > >>> index e3b541db4e36..dd0f4d3f64b6 100644 > >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > >>> @@ -45,7 +45,7 @@ namespace {{ns}} { > >>> {%- endif %} > >>> > >>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) > >>> - : IPAProxy(ipam), running_(false), > >>> + : IPAProxy(ipam), state_(Stopped), > >>> isolate_(isolate), seq_(0) > >>> { > >>> LOG(IPAProxy, Debug) > >>> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > >>> > >>> return {{ "_ret" if method|method_return_value != "void" }}; > >>> {%- elif method.mojom_name == "start" %} > >>> - running_ = true; > >>> + state_ = Running; > >>> thread_.start(); > >>> > >>> {{ "return " if method|method_return_value != "void" -}} > >>> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > >>> {%- endfor -%} > >>> ); > >>> {% elif method|is_async %} > >>> - ASSERT(running_); > >>> + ASSERT(state_ == Running); > >>> proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued, > >>> {%- for param in method|method_param_names -%} > >>> {{param}}{{- ", " if not loop.last}} > >>> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > >>> {% for method in interface_event.methods %} > >>> {{proxy_funcs.func_sig(proxy_name, method, "Thread")}} > >>> { > >>> - ASSERT(running_); > >>> + ASSERT(state_ != Stopped); > >>> {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}}); > >>> } > >> > >> As mentioned on irc, I think we need to copy these over to the IPC > >> versions too, but that can be done on top. > > I'm not sure I understand this here ... Paul meant that asserts in the IPC case would be useful. > >>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > >>> index efb09a180b90..e33c26492d91 100644 > >>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > >>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl > >>> @@ -104,7 +104,13 @@ private: > >>> {{interface_name}} *ipa_; > >>> }; > >>> > >>> - bool running_; > >>> + enum {{proxy_name}}States { > >>> + Stopped, > >>> + Stopping, > >>> + Running, > >>> + }; > >>> + enum {{proxy_name}}States state_; > >>> + > >> > >> Since these are the same for all IPAProxies, should we put them in > >> ipa_proxy.h in the base IPAProxy class instead? > > > > I like this. > > I started with that and then moved it here for some reason, that I can > no longer recall. > > But keeping it common is better I agree, so I'll move it back. > > >> With or without the suggested change, > >> > >> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > >> > >>> Thread thread_; > >>> ThreadProxy proxy_; > >>> std::unique_ptr<{{interface_name}}> ipa_; > >>> diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > >>> index 8addc2fad0a8..09394a4fec83 100644 > >>> --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > >>> +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl > >>> @@ -23,9 +23,12 @@ > >>> # \brief Generate function body for IPA stop() function for thread > >>> #} > >>> {%- macro stop_thread_body() -%} > >>> - if (!running_) > >>> + ASSERT(state_ != Stopping); > >>> + if (state_ != Running) > >>> return; > >>> > >>> + state_ = Stopping; > >>> + > >>> proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking); > >>> > >>> thread_.exit(); > >>> @@ -33,7 +36,7 @@ > >>> > >>> Thread::current()->dispatchMessages(Message::Type::InvokeMessage); > >>> > >>> - running_ = false; > >>> + state_ = Stopped; > >>> {%- endmacro -%} > >>> > >>>
Hi Laurent, On 26/03/2021 12:34, Laurent Pinchart wrote: > Hi Kieran, > > On Fri, Mar 26, 2021 at 12:27:46PM +0000, Kieran Bingham wrote: >> On 26/03/2021 02:52, Laurent Pinchart wrote: >>> On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote: >>>> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote: >>>>> Asynchronous tasks can only be submitted while the IPA is running. >>>>> >>>>> Further more, the shutdown sequence can not be tracked with a simple >>>>> running flag. We can also be in the state 'Stopping' where we have not >>>>> yet completed all events, but we must not commence anything new. >>>>> >>>>> Refactor the running_ boolean into a stateful enum to track this. >>>> >>>> Ooh, good idea!>> >>>>> >>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>> --- >>>>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++---- >>>>> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++- >>>>> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++-- >>>>> 3 files changed, 16 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >>>>> index e3b541db4e36..dd0f4d3f64b6 100644 >>>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >>>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl >>>>> @@ -45,7 +45,7 @@ namespace {{ns}} { >>>>> {%- endif %} >>>>> >>>>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) >>>>> - : IPAProxy(ipam), running_(false), >>>>> + : IPAProxy(ipam), state_(Stopped), >>>>> isolate_(isolate), seq_(0) >>>>> { >>>>> LOG(IPAProxy, Debug) >>>>> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) >>>>> >>>>> return {{ "_ret" if method|method_return_value != "void" }}; >>>>> {%- elif method.mojom_name == "start" %} >>>>> - running_ = true; >>>>> + state_ = Running; >>>>> thread_.start(); >>>>> >>>>> {{ "return " if method|method_return_value != "void" -}} >>>>> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) >>>>> {%- endfor -%} >>>>> ); >>>>> {% elif method|is_async %} >>>>> - ASSERT(running_); >>>>> + ASSERT(state_ == Running); >>>>> proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued, >>>>> {%- for param in method|method_param_names -%} >>>>> {{param}}{{- ", " if not loop.last}} >>>>> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) >>>>> {% for method in interface_event.methods %} >>>>> {{proxy_funcs.func_sig(proxy_name, method, "Thread")}} >>>>> { >>>>> - ASSERT(running_); >>>>> + ASSERT(state_ != Stopped); >>>>> {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}}); >>>>> } >>>> >>>> As mentioned on irc, I think we need to copy these over to the IPC >>>> versions too, but that can be done on top. >> >> I'm not sure I understand this here ... > > Paul meant that asserts in the IPC case would be useful. I can *read* the sentence ... but not interpret the location that needs updating ;-) Can you provide additional context please? Otherwise I'll leave this out, and let someone tackle it on top.
Hi Kieran, On Mon, Mar 29, 2021 at 12:31:49PM +0100, Kieran Bingham wrote: > On 26/03/2021 12:34, Laurent Pinchart wrote: > > On Fri, Mar 26, 2021 at 12:27:46PM +0000, Kieran Bingham wrote: > >> On 26/03/2021 02:52, Laurent Pinchart wrote: > >>> On Fri, Mar 26, 2021 at 11:50:10AM +0900, paul.elder@ideasonboard.com wrote: > >>>> On Thu, Mar 25, 2021 at 01:42:21PM +0000, Kieran Bingham wrote: > >>>>> Asynchronous tasks can only be submitted while the IPA is running. > >>>>> > >>>>> Further more, the shutdown sequence can not be tracked with a simple > >>>>> running flag. We can also be in the state 'Stopping' where we have not > >>>>> yet completed all events, but we must not commence anything new. > >>>>> > >>>>> Refactor the running_ boolean into a stateful enum to track this. > >>>> > >>>> Ooh, good idea!>> > >>>>> > >>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>>>> --- > >>>>> .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++---- > >>>>> .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++- > >>>>> .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++-- > >>>>> 3 files changed, 16 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > >>>>> index e3b541db4e36..dd0f4d3f64b6 100644 > >>>>> --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > >>>>> +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl > >>>>> @@ -45,7 +45,7 @@ namespace {{ns}} { > >>>>> {%- endif %} > >>>>> > >>>>> {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) > >>>>> - : IPAProxy(ipam), running_(false), > >>>>> + : IPAProxy(ipam), state_(Stopped), > >>>>> isolate_(isolate), seq_(0) > >>>>> { > >>>>> LOG(IPAProxy, Debug) > >>>>> @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > >>>>> > >>>>> return {{ "_ret" if method|method_return_value != "void" }}; > >>>>> {%- elif method.mojom_name == "start" %} > >>>>> - running_ = true; > >>>>> + state_ = Running; > >>>>> thread_.start(); > >>>>> > >>>>> {{ "return " if method|method_return_value != "void" -}} > >>>>> @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > >>>>> {%- endfor -%} > >>>>> ); > >>>>> {% elif method|is_async %} > >>>>> - ASSERT(running_); > >>>>> + ASSERT(state_ == Running); > >>>>> proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued, > >>>>> {%- for param in method|method_param_names -%} > >>>>> {{param}}{{- ", " if not loop.last}} > >>>>> @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) > >>>>> {% for method in interface_event.methods %} > >>>>> {{proxy_funcs.func_sig(proxy_name, method, "Thread")}} > >>>>> { > >>>>> - ASSERT(running_); > >>>>> + ASSERT(state_ != Stopped); > >>>>> {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}}); > >>>>> } > >>>> > >>>> As mentioned on irc, I think we need to copy these over to the IPC > >>>> versions too, but that can be done on top. > >> > >> I'm not sure I understand this here ... > > > > Paul meant that asserts in the IPC case would be useful. > > I can *read* the sentence ... but not interpret the location that needs > updating ;-) Sorry :-) > Can you provide additional context please? > > Otherwise I'll leave this out, and let someone tackle it on top. It can go on top, no issue. It should actually go on top, as I expect we may need a different state machine for the IPC case, given that we need to keep processing messages while waiting for the reply to sync calls, and that will be a larger development.
diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl index e3b541db4e36..dd0f4d3f64b6 100644 --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.cpp.tmpl @@ -45,7 +45,7 @@ namespace {{ns}} { {%- endif %} {{proxy_name}}::{{proxy_name}}(IPAModule *ipam, bool isolate) - : IPAProxy(ipam), running_(false), + : IPAProxy(ipam), state_(Stopped), isolate_(isolate), seq_(0) { LOG(IPAProxy, Debug) @@ -155,7 +155,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) return {{ "_ret" if method|method_return_value != "void" }}; {%- elif method.mojom_name == "start" %} - running_ = true; + state_ = Running; thread_.start(); {{ "return " if method|method_return_value != "void" -}} @@ -173,7 +173,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) {%- endfor -%} ); {% elif method|is_async %} - ASSERT(running_); + ASSERT(state_ == Running); proxy_.invokeMethod(&ThreadProxy::{{method.mojom_name}}, ConnectionTypeQueued, {%- for param in method|method_param_names -%} {{param}}{{- ", " if not loop.last}} @@ -226,7 +226,7 @@ void {{proxy_name}}::recvMessage(const IPCMessage &data) {% for method in interface_event.methods %} {{proxy_funcs.func_sig(proxy_name, method, "Thread")}} { - ASSERT(running_); + ASSERT(state_ != Stopped); {{method.mojom_name}}.emit({{method.parameters|params_comma_sep}}); } diff --git a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl index efb09a180b90..e33c26492d91 100644 --- a/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl +++ b/utils/ipc/generators/libcamera_templates/module_ipa_proxy.h.tmpl @@ -104,7 +104,13 @@ private: {{interface_name}} *ipa_; }; - bool running_; + enum {{proxy_name}}States { + Stopped, + Stopping, + Running, + }; + enum {{proxy_name}}States state_; + Thread thread_; ThreadProxy proxy_; std::unique_ptr<{{interface_name}}> ipa_; diff --git a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl index 8addc2fad0a8..09394a4fec83 100644 --- a/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl +++ b/utils/ipc/generators/libcamera_templates/proxy_functions.tmpl @@ -23,9 +23,12 @@ # \brief Generate function body for IPA stop() function for thread #} {%- macro stop_thread_body() -%} - if (!running_) + ASSERT(state_ != Stopping); + if (state_ != Running) return; + state_ = Stopping; + proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking); thread_.exit(); @@ -33,7 +36,7 @@ Thread::current()->dispatchMessages(Message::Type::InvokeMessage); - running_ = false; + state_ = Stopped; {%- endmacro -%}
Asynchronous tasks can only be submitted while the IPA is running. Further more, the shutdown sequence can not be tracked with a simple running flag. We can also be in the state 'Stopping' where we have not yet completed all events, but we must not commence anything new. Refactor the running_ boolean into a stateful enum to track this. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- .../libcamera_templates/module_ipa_proxy.cpp.tmpl | 8 ++++---- .../libcamera_templates/module_ipa_proxy.h.tmpl | 8 +++++++- .../generators/libcamera_templates/proxy_functions.tmpl | 7 +++++-- 3 files changed, 16 insertions(+), 7 deletions(-)