* Using a struct[enum] as an sdbusplus D-Bus method arg?
@ 2020-01-08 19:32 Matt Spinler
2020-01-09 16:22 ` Patrick Williams
0 siblings, 1 reply; 9+ messages in thread
From: Matt Spinler @ 2020-01-08 19:32 UTC (permalink / raw)
To: OpenBMC Maillist
Hi,
I'm designing a phosphor-dbus-interfaces D-Bus method where I think the
ideal interface has YAML like:
- name : CreateStuff
parameters:
- name: Data
type: array[struct[enum[self.Types], uint32]]
But that doesn't compile using an enum in the struct, and fails because
sdbus++ creates code like:
server.cpp:
// Uses enum<self.Types>
std::vector<std::tuple<enum<self.Types>, uint8_t>> data{};
m.read(data);
auto o = static_cast<Create*>(context);
o->createStuff(data);
What I'm trying to do is a valid D-Bus definition, correct? Or should I
just use a string instead
of an enum?
It doesn't seem very straightforward to fix, as the vector that gets
passed to o->createStuff()
is a std::vector<std::tuple<Types, uint32_t>>, but the vector passed to
m.read() is a
std::vector<std::tuple<std::string, uint32_t>>.
Any thoughts?
Thanks!
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Using a struct[enum] as an sdbusplus D-Bus method arg?
2020-01-08 19:32 Using a struct[enum] as an sdbusplus D-Bus method arg? Matt Spinler
@ 2020-01-09 16:22 ` Patrick Williams
2020-01-09 17:25 ` Matt Spinler
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Williams @ 2020-01-09 16:22 UTC (permalink / raw)
To: Matt Spinler; +Cc: OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]
On Wed, Jan 08, 2020 at 01:32:49PM -0600, Matt Spinler wrote:
Hi Matt,
> I'm designing a phosphor-dbus-interfaces D-Bus method where I think the
> ideal interface has YAML like:
>
> - name : CreateStuff
> parameters:
> - name: Data
> type: array[struct[enum[self.Types], uint32]]
>
> But that doesn't compile using an enum in the struct, and fails because
> sdbus++ creates code like:
>
> server.cpp:
>
> // Uses enum<self.Types>
> std::vector<std::tuple<enum<self.Types>, uint8_t>> data{};
> m.read(data);
> auto o = static_cast<Create*>(context);
> o->createStuff(data);
>
> What I'm trying to do is a valid D-Bus definition, correct? Or should I just
> use a string instead
> of an enum?
Enums are not actually a DBus concept, but something we added to
sdbusplus specifically. What happens is that at the DBus level we send
them between processes as fully-qualified strings[1]. In the sdbusplus
generated code we know that this was defined in the YAML as an enum and
so we look the string up in a map<string, enum> to get you the enum
value (and throw an exception if an invalid enum was given).
I wouldn't recommend using a string instead because you are just
bypassing this enum-string checking code.
> It doesn't seem very straightforward to fix, as the vector that gets passed
> to o->createStuff()
> is a std::vector<std::tuple<Types, uint32_t>>, but the vector passed to
> m.read() is a
> std::vector<std::tuple<std::string, uint32_t>>.
This is a bug / missing feature in the header generator. I'd have to
look into the code but it seems like it is not re-cursing inside the
tuple correctly. We handle this case fine for raw enums. The
definition of data's type looks like a start of the problem
(enum<self.Types> doesn't seem correct to me). It's possible though
that the m.read generated function is wrong in that it is missing the
conversion from self.Types <-> std::string.
[1] A fully-qualified string is something like
xyz.openbmc-project.InterfaceClass.EnumType.EnumValue.
--
Patrick Williams
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Using a struct[enum] as an sdbusplus D-Bus method arg?
2020-01-09 16:22 ` Patrick Williams
@ 2020-01-09 17:25 ` Matt Spinler
2020-01-09 19:03 ` Patrick Williams
0 siblings, 1 reply; 9+ messages in thread
From: Matt Spinler @ 2020-01-09 17:25 UTC (permalink / raw)
To: Patrick Williams; +Cc: OpenBMC Maillist
On 1/9/2020 10:22 AM, Patrick Williams wrote:
> On Wed, Jan 08, 2020 at 01:32:49PM -0600, Matt Spinler wrote:
>
> Hi Matt,
>
>> I'm designing a phosphor-dbus-interfaces D-Bus method where I think the
>> ideal interface has YAML like:
>>
>> - name : CreateStuff
>> parameters:
>> - name: Data
>> type: array[struct[enum[self.Types], uint32]]
>>
>> But that doesn't compile using an enum in the struct, and fails because
>> sdbus++ creates code like:
>>
>> server.cpp:
>>
>> // Uses enum<self.Types>
>> std::vector<std::tuple<enum<self.Types>, uint8_t>> data{};
>> m.read(data);
>> auto o = static_cast<Create*>(context);
>> o->createStuff(data);
>>
>> What I'm trying to do is a valid D-Bus definition, correct? Or should I just
>> use a string instead
>> of an enum?
> Enums are not actually a DBus concept, but something we added to
> sdbusplus specifically. What happens is that at the DBus level we send
> them between processes as fully-qualified strings[1]. In the sdbusplus
> generated code we know that this was defined in the YAML as an enum and
> so we look the string up in a map<string, enum> to get you the enum
> value (and throw an exception if an invalid enum was given).
>
> I wouldn't recommend using a string instead because you are just
> bypassing this enum-string checking code.
>
>> It doesn't seem very straightforward to fix, as the vector that gets passed
>> to o->createStuff()
>> is a std::vector<std::tuple<Types, uint32_t>>, but the vector passed to
>> m.read() is a
>> std::vector<std::tuple<std::string, uint32_t>>.
> This is a bug / missing feature in the header generator. I'd have to
> look into the code but it seems like it is not re-cursing inside the
> tuple correctly. We handle this case fine for raw enums. The
> definition of data's type looks like a start of the problem
> (enum<self.Types> doesn't seem correct to me). It's possible though
> that the m.read generated function is wrong in that it is missing the
> conversion from self.Types <-> std::string.
Since the vectors are of different types, I think the code generated
code would have to
do the equivalent of?
std::vector<std::tuple<std::string, uint8_t>> data{};
m.read(data);
std::vector<std::tuple<Types, uint8_t>> apiData{};
for (const auto& [arg0, arg1]: data)
apiData.emplace_back(convertStringToEnum(arg0), arg1))
o->createStuff(apiData);
That's a pretty big change from what's there today. Think that's possible
to do generically?
> [1] A fully-qualified string is something like
> xyz.openbmc-project.InterfaceClass.EnumType.EnumValue.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Using a struct[enum] as an sdbusplus D-Bus method arg?
2020-01-09 17:25 ` Matt Spinler
@ 2020-01-09 19:03 ` Patrick Williams
2020-01-13 18:59 ` Matt Spinler
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Williams @ 2020-01-09 19:03 UTC (permalink / raw)
To: Matt Spinler; +Cc: OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]
On Thu, Jan 09, 2020 at 11:25:24AM -0600, Matt Spinler wrote:
>
>
> On 1/9/2020 10:22 AM, Patrick Williams wrote:
> > On Wed, Jan 08, 2020 at 01:32:49PM -0600, Matt Spinler wrote:
> Since the vectors are of different types, I think the code generated code
> would have to
> do the equivalent of?
>
> std::vector<std::tuple<std::string, uint8_t>> data{};
> m.read(data);
>
> std::vector<std::tuple<Types, uint8_t>> apiData{};
>
> for (const auto& [arg0, arg1]: data)
> apiData.emplace_back(convertStringToEnum(arg0), arg1))
> o->createStuff(apiData);
> That's a pretty big change from what's there today. Think that's possible
> to do generically?
>
It seems like that is something that should have been done in the
`message::read` function. First, since your initial data type is wrong
(that funny enum<...>), I think we need to figure out why this function
isn't working correctly:
https://github.com/openbmc/sdbusplus/blob/master/tools/sdbusplus/property.py#L72
It was suppose to have handled, through recursion, enums. I would have
expected data's type to be vector<tuple<Interface::Types, uint8_t>>.
After that, I suspect you'll get compile errors that m.read doesn't know
how to handle an Interface::Types enum type. This would require some
C++ template magic, but I think it could be handled there. We need to
teach m.read to be able to call convertToMessage and convertFromMessage
if those exist. I don't know why we originally did that lifting in the
from the generated code, except maybe that enums were bolted on later
and never fully complete?
--
Patrick Williams
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Using a struct[enum] as an sdbusplus D-Bus method arg?
2020-01-09 19:03 ` Patrick Williams
@ 2020-01-13 18:59 ` Matt Spinler
2020-01-14 17:41 ` Patrick Williams
0 siblings, 1 reply; 9+ messages in thread
From: Matt Spinler @ 2020-01-13 18:59 UTC (permalink / raw)
To: Patrick Williams; +Cc: OpenBMC Maillist
On 1/9/2020 1:03 PM, Patrick Williams wrote:
> On Thu, Jan 09, 2020 at 11:25:24AM -0600, Matt Spinler wrote:
>> On 1/9/2020 10:22 AM, Patrick Williams wrote:
>>> On Wed, Jan 08, 2020 at 01:32:49PM -0600, Matt Spinler wrote:
>> Since the vectors are of different types, I think the code generated code
>> would have to
>> do the equivalent of?
>>
>> std::vector<std::tuple<std::string, uint8_t>> data{};
>> m.read(data);
>>
>> std::vector<std::tuple<Types, uint8_t>> apiData{};
>>
>> for (const auto& [arg0, arg1]: data)
>> apiData.emplace_back(convertStringToEnum(arg0), arg1))
>> o->createStuff(apiData);
>> That's a pretty big change from what's there today. Think that's possible
>> to do generically?
>>
> It seems like that is something that should have been done in the
> `message::read` function. First, since your initial data type is wrong
> (that funny enum<...>), I think we need to figure out why this function
> isn't working correctly:
>
> https://github.com/openbmc/sdbusplus/blob/master/tools/sdbusplus/property.py#L72
>
> It was suppose to have handled, through recursion, enums. I would have
> expected data's type to be vector<tuple<Interface::Types, uint8_t>>.
> After that, I suspect you'll get compile errors that m.read doesn't know
> how to handle an Interface::Types enum type. This would require some
> C++ template magic, but I think it could be handled there. We need to
> teach m.read to be able to call convertToMessage and convertFromMessage
> if those exist. I don't know why we originally did that lifting in the
> from the generated code, except maybe that enums were bolted on later
> and never fully complete?
>
To go from enums to strings, there is a convertForMessage() function,
but to go from
strings back to the enums, which is what we need, the function has the
enum name in it:
convert<enum name>FromString(const std::string& s);
I don't think it's possible for message::read() to know that?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Using a struct[enum] as an sdbusplus D-Bus method arg?
2020-01-13 18:59 ` Matt Spinler
@ 2020-01-14 17:41 ` Patrick Williams
2020-01-29 3:07 ` Patrick Williams
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Williams @ 2020-01-14 17:41 UTC (permalink / raw)
To: Matt Spinler; +Cc: OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]
On Mon, Jan 13, 2020 at 12:59:26PM -0600, Matt Spinler wrote:
> To go from enums to strings, there is a convertForMessage() function, but to
> go from
> strings back to the enums, which is what we need, the function has the enum
> name in it:
> convert<enum name>FromString(const std::string& s);
>
> I don't think it's possible for message::read() to know that?
This needs a pretty simple refactoring to rename the
convert<enum>FromString function to a template function.
// in a header somewhere to forward declare.
template <typename T> T convertFromString() = delete;
// in the generated code for each enum.
template <> enum_type convertFromString<enum_type>() { ... }
The message::read will be able map message::read<enum_type> to
convertFromString<enum_type>. There is probably some SFINAE needed
using is_enum[1] so that we don't need to generate each
message::<enum_type> specialization but can use a generic one for all
enums.
[1] https://en.cppreference.com/w/cpp/types/is_enum
--
Patrick Williams
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Using a struct[enum] as an sdbusplus D-Bus method arg?
2020-01-14 17:41 ` Patrick Williams
@ 2020-01-29 3:07 ` Patrick Williams
2020-01-29 15:08 ` Matt Spinler
0 siblings, 1 reply; 9+ messages in thread
From: Patrick Williams @ 2020-01-29 3:07 UTC (permalink / raw)
To: Matt Spinler; +Cc: OpenBMC Maillist
On Tue, Jan 14, 2020 at 11:41:17AM -0600, Patrick Williams wrote:
> On Mon, Jan 13, 2020 at 12:59:26PM -0600, Matt Spinler wrote:
> This needs a pretty simple refactoring to rename the
> convert<enum>FromString function to a template function.
>
> --
> Patrick Williams
This probably wasn't as simple as I lead on with this previous email,
but patches are up for sdbusplus ending with this change:
Ib142482de90572e1bda2f3658f6aeec201c043de
https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/28859
With this commit sequence you should be able to read / write any
container with sdbusplus-enums in it with the message::read / append
functions, which as a side-effect means the generated classes no
longer need all the conversion hoops they use to jump through.
--
Patrick Williams
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Using a struct[enum] as an sdbusplus D-Bus method arg?
2020-01-29 3:07 ` Patrick Williams
@ 2020-01-29 15:08 ` Matt Spinler
2020-02-04 21:55 ` Patrick Williams
0 siblings, 1 reply; 9+ messages in thread
From: Matt Spinler @ 2020-01-29 15:08 UTC (permalink / raw)
To: Patrick Williams; +Cc: OpenBMC Maillist
On 1/28/2020 9:07 PM, Patrick Williams wrote:
> On Tue, Jan 14, 2020 at 11:41:17AM -0600, Patrick Williams wrote:
>> On Mon, Jan 13, 2020 at 12:59:26PM -0600, Matt Spinler wrote:
>> This needs a pretty simple refactoring to rename the
>> convert<enum>FromString function to a template function.
>>
>> --
>> Patrick Williams
> This probably wasn't as simple as I lead on with this previous email,
> but patches are up for sdbusplus ending with this change:
> Ib142482de90572e1bda2f3658f6aeec201c043de
>
> https://gerrit.openbmc-project.xyz/c/openbmc/sdbusplus/+/28859
>
> With this commit sequence you should be able to read / write any
> container with sdbusplus-enums in it with the message::read / append
> functions, which as a side-effect means the generated classes no
> longer need all the conversion hoops they use to jump through.
>
Thanks!
I tried to build a flash (wspoon) image, and in the server.hpp generated
for my interface,
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-dbus-interfaces/+/26544,
it had:
#include <std/vector<std/message/tuple<FFDCFormat, uint8_t, uint8_t,
sdbusplus.hpp>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Using a struct[enum] as an sdbusplus D-Bus method arg?
2020-01-29 15:08 ` Matt Spinler
@ 2020-02-04 21:55 ` Patrick Williams
0 siblings, 0 replies; 9+ messages in thread
From: Patrick Williams @ 2020-02-04 21:55 UTC (permalink / raw)
To: Matt Spinler; +Cc: OpenBMC Maillist
[-- Attachment #1: Type: text/plain, Size: 458 bytes --]
On Wed, Jan 29, 2020 at 09:08:28AM -0600, Matt Spinler wrote:
> Thanks!
> I tried to build a flash (wspoon) image, and in the server.hpp generated for
> my interface,
> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-dbus-interfaces/+/26544,
> it had:
>
> #include <std/vector<std/message/tuple<FFDCFormat, uint8_t, uint8_t,
> sdbusplus.hpp>
Latest version of the changes should fix this #include issue now, Matt.
--
Patrick Williams
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-02-04 21:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-08 19:32 Using a struct[enum] as an sdbusplus D-Bus method arg? Matt Spinler
2020-01-09 16:22 ` Patrick Williams
2020-01-09 17:25 ` Matt Spinler
2020-01-09 19:03 ` Patrick Williams
2020-01-13 18:59 ` Matt Spinler
2020-01-14 17:41 ` Patrick Williams
2020-01-29 3:07 ` Patrick Williams
2020-01-29 15:08 ` Matt Spinler
2020-02-04 21:55 ` Patrick Williams
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.