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

[BUG] Devmod Modules First Index is Always 0 #696

Open
ben-krieger opened this issue Sep 11, 2024 · 5 comments
Open

[BUG] Devmod Modules First Index is Always 0 #696

ben-krieger opened this issue Sep 11, 2024 · 5 comments

Comments

@ben-krieger
Copy link
Member

Describe the bug
Service info sent in message 68 for devmod looks as below.

[false, [["devmod:active", h'f5'], ["devmod:os", h'654c696e7578'], ["devmod:arch", h'65616d643634'], ["devmod:version", h'781a362e362e34312d30333532302d67643364373766313566383432'], ["devmod:device", h'6e46444f2d5072692d446576696365'], ["devmod:sep", h'613a'], ["devmod:bin", h'65616d643634'], ["devmod:nummodules", h'02'], ["devmod:modules", h'830001666465766d6f64'], ["devmod:modules", h'8300016766646f5f737973']]]

There are two "devmod:modules" messages, containing bstr data:

[0, 1, "devmod"]
[0, 1, "fdo_sys"]

The spec is unclear, only stating that "the first element is an integer from zero to devmod:nummodules", but spec authors have verified to me that the purpose of the first element is "start index".

This means that the first message is interpreted as "from index 0, insert 1 element." Therefore the second message should be "from index 1, insert 1 element" and then you have a 2 element list, matching nummodules.

To Reproduce

  • Perform TO2 with the demo device.jar

Expected behavior

Modules bstr data should be

[0, 1, "devmod"]
[1, 1, "fdo_sys"]
@GeofCooper
Copy link

GeofCooper commented Sep 11, 2024

Example given in the spec is: [uint, uint, tstr1, tstr2, ...]

So you can return the modules one at a time:

...[n, 1, name][n+1, 1, name]...
e.g., [0, 1, "devmod][1, 1, "fdo_sys"]... as you said

or in a larger list
...[10, 1, mod1, mod2, ..., mod10]...
[0, 2, "devmod", "fdo_sys"]

The latter is the original intent, actually.
The reason you don't just list all the modules is that it might not fit into a single message.
So the format gives you a way to describe as many modules as you have room for.
I never intended that you list them one per array, although this is legal.

I also sort of expected that you would list them in some order, but this is not mandated. In fact, you can list them differently on subsequent calls. But the intent is that you assign some order to all your modules, then use this mechanism to chop up the list into chunks that fit into serviceinfo.

@GeofCooper
Copy link

I guess the text actually permits you to use the same number for each element, so if you have modules a, b, c, d, you could return

[0, 1, a]
[0, 2, b, c]
[0, 3, d]

and the idea is that you are reordering the list each time. I would not advise this particular behavior, and I'll try to clarify in 1.2.

@GeofCooper
Copy link

Here is my proposed change to the text in FDO 1.2:

Enumerates the names of modules supported by this [FIDOIOT] Device.
The device internally assigns an index to each module in the
range [0...nummodules-1]. The list is sent in one or more arrays
containing [index, N, modname1, ... modnameN]. The intent is to send
the full list as a single array, but breaking the list is acceptable,
and may be necessary to allow it to fit into ServiceInfo messages.

@ben-krieger
Copy link
Member Author

Here is my proposed change to the text in FDO 1.2:

Enumerates the names of modules supported by this [FIDOIOT] Device. The device internally assigns an index to each module in the range [0...nummodules-1]. The list is sent in one or more arrays containing [index, N, modname1, ... modnameN]. The intent is to send the full list as a single array, but breaking the list is acceptable, and may be necessary to allow it to fit into ServiceInfo messages.

Only thing I would change is that [index, N, modname1, ...modnameN] is more clear as [i, N, modname(i), ...modname(i+N)] or similar. And maybe there's still a way to make it clear that you shouldn't send any modname(i+N) twice.

@GeofCooper
Copy link

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants