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

ng_net: changed semantic of get and set command slightly #2598

Merged
merged 3 commits into from
Mar 16, 2015

Conversation

haukepetersen
Copy link
Contributor

When working with netdev/netapi, I found the get function over complicated. This PR simplifies the semantics a little by making use of the netdev->get() functions return value (like every Linux programmer would expect)...

So when reading for example a network address from a device, the max_length parameter can be used to specify how many bytes should be read. The device driver can then take from this number which address it should read, e.g. a 802.15.4 device returns the short address if 2 byte are asked for and the long address if 8 byte are asked for...

@haukepetersen haukepetersen changed the title net: changed semantic of get command slightly ng_net: changed semantic of get command slightly Mar 13, 2015
@haukepetersen haukepetersen added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation NSTF labels Mar 13, 2015
if (argc < 2) {
printf("usage: %s IF [ADDR]\n", argv[0]);
if (argc < 3) {
printf("usage: %s IF ADDR_LEN (in byte)\n", argv[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the user get this information? Why do we need it in addition to MAX_ADDR_LEN?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter is basically used to distinguish between different address types (short/long). If the user does not specify any length, there would be no way in knowing which address the user wants to know...

@miri64
Copy link
Member

miri64 commented Mar 13, 2015

We also should note, that most drivers will error if max_len != sizeof(type) for scalar values. Otherwise the implementation will get quite hairy ;-).

@miri64
Copy link
Member

miri64 commented Mar 13, 2015

Apart from the split of the shell command, which seems a bit nonsensical to me 👍

@miri64 miri64 added this to the Network Stack Task Force milestone Mar 13, 2015
@miri64
Copy link
Member

miri64 commented Mar 13, 2015

(it also would collide with #2581 ;-P)

@miri64 miri64 changed the title ng_net: changed semantic of get command slightly ng_net: changed semantic of get and set command slightly Mar 13, 2015
@haukepetersen
Copy link
Contributor Author

The split is necessary, as both addr_set and addr_get have 3 parameters. Alternatively I could introduce a forth parameter, but this seemed easier to me (as #2581 does change this again anyway?!)

saw #2581 only after I did these changes...

@miri64
Copy link
Member

miri64 commented Mar 13, 2015

The split is necessary, as both addr_set and addr_get have 3 parameters. Alternatively I could introduce a forth parameter, but this seemed easier to me (as #2581 does change this again anyway?!)

I don't get why you need 3 parameters for addr_get. We have MAX_ADDR_LEN to determine max_len. Why does the user has to type in an address length manually if there is still a check directly afterwards if it is > MAX_ADDR_LEN (which yields an error).

return;
}
res = ng_netapi_set(dev, NETCONF_OPT_ADDRESS, 0, addr, addr_len);
if (res == addr_len) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is addr_len the maximum length now or the exact length? I think your change to set(), though understandable to keep it same to get(), is unclear here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay… I guess I have to be clearer here: What I normally do with array-like values like addresses: I call ng_netapi_set(dev, NETCONF_OPT_ADDRSS, 0, addr, sizeof(addr)), where addr is an array of arbitrary length. Would this be an error case? If yes, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The meaning of the len parameter is quite straight forward in my opinion. When calling set, you specify the length of the given address (as you state in your example above). For interfaces, that support addresses of different length, this parameter can be used to detect which address should be set (short vs. long address). So it is possible to set both addresses with the same function.

When calling get, the max_len parameter is used in the same way. If I would just give the MAX_ADDR_LEN, the driver would have no means of finding out, which address is actually requested. By setting max_len to say 2 or 8, the driver can read from this context, which address the user wants to read.

So for set, the len parameter defines the exact length of the data to be written to the device. For get, the max_len parameter defines for one the maximum number of bytes that can be written to the data buffer, but gives also a context for the amount of data that is expected.

Clearer now?

miri64 added a commit to miri64/RIOT that referenced this pull request Mar 13, 2015
miri64 added a commit to miri64/RIOT that referenced this pull request Mar 13, 2015
miri64 added a commit to miri64/RIOT that referenced this pull request Mar 13, 2015
@haukepetersen
Copy link
Contributor Author

The third parameter for addr_get is important to specify the expected length of the address that is read - this is important to differentiate between different device address (short vs. long address). Without this parameter, there would be no means of telling the driver, which address you want to get.

The MAX_ADDR_LEN macro is simply there to define the internal array size that is used to save the address internally - it's value is chosen arbitrarily...

@haukepetersen
Copy link
Contributor Author

After re-consideration, it was decided that the idea of a unified shell command might not be as good as I thought. So I took the changes to the netif shell commands out of this PR...

@haukepetersen
Copy link
Contributor Author

The reason is that is a times difficult to know the exact addr length, when using the get command in an automated environment (as for example the ifconfig shell command)...

@miri64
Copy link
Member

miri64 commented Mar 16, 2015

ACK. Waiting for travis to be happy :-)

@haukepetersen
Copy link
Contributor Author

Travis is happy. And go.

haukepetersen added a commit that referenced this pull request Mar 16, 2015
ng_net: changed semantic of get and set command slightly
@haukepetersen haukepetersen merged commit da44b0d into RIOT-OS:master Mar 16, 2015
@haukepetersen haukepetersen deleted the ng_opt_getset branch March 16, 2015 20:05
miri64 added a commit to miri64/RIOT that referenced this pull request Mar 17, 2015
miri64 added a commit to miri64/RIOT that referenced this pull request Mar 19, 2015
miri64 added a commit to miri64/RIOT that referenced this pull request Mar 19, 2015
miri64 added a commit to miri64/RIOT that referenced this pull request Mar 24, 2015
@miri64 miri64 added the Area: network Area: Networking label Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants