Skip to content

kw2xrf: set tx_power in gnrc_netdev_t#5834

Merged
jfischer-no merged 1 commit intoRIOT-OS:masterfrom
smlng:pr/drivers/kw2xrf/set_tx_power
Sep 22, 2016
Merged

kw2xrf: set tx_power in gnrc_netdev_t#5834
jfischer-no merged 1 commit intoRIOT-OS:masterfrom
smlng:pr/drivers/kw2xrf/set_tx_power

Conversation

@smlng
Copy link
Copy Markdown
Member

@smlng smlng commented Sep 9, 2016

without this patch, tx_power is directly set on the device but not in
gnrc_netdev_t. Thus, calling ifconfig in shell shows tx_power always
at 0dBm, never showing the correct, current value.

Btw. verified by testing, 5 min ago 😄

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 9, 2016

Please specify in the commit message which driver you are talking about ;-)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 9, 2016

(and in the PR title)

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Area: drivers Area: Device drivers labels Sep 9, 2016
@cgundogan cgundogan changed the title set tx_power in gnrc_netdev_t kw2xrf: set tx_power in gnrc_netdev_t Sep 9, 2016
@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Sep 9, 2016

the same is missing for the at86rf driver

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 9, 2016

@cgundogan but there the option getter gets the option directly from the device.

@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Sep 9, 2016

@miri64 but AFAIK this is what should not happen. Isn't the gnrc_netdev_t struct there to cache the values and not ask the device everytime?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 9, 2016

There are no rules to that. I would argue, if the connection to the device is fast enough its worth saving the RAM to get the value directly from the device.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 9, 2016

(also the gnrc_netdev_t struct isn't supposed to save any of that data [especially not for netdev2 where there is netdev2_t]. Maybe the internal struct but its main purpose also is just to store internal state for the system to interact with the device.)

@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Sep 9, 2016

AFAIR the incentive was to reduce the register access, which is basically an SPI access

@cgundogan cgundogan added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Sep 9, 2016
@cgundogan
Copy link
Copy Markdown
Member

cgundogan commented Sep 9, 2016 •
edited
Loading

needs some modifications in kw2xrf_set_tx_power (offline discussion with @smlng)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 9, 2016

These changes should also be noted for #5469.

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Sep 9, 2016

I added checks to verify that the tx_power passed by e.g. ifconfig shell command are valid, that is in allowed range by driver.

Regarding the discussion whether to cache values in gnrc_netdev_t or always ask the device directly: why is there a tx_power attribute in the struct anyway then? If rules are not cache those, then delete the attribute so nobody can use it.

@smlng smlng added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Sep 9, 2016
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Sep 9, 2016 •
edited
Loading

and again: verified by live testing on hardware

(Edit): works!

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Sep 9, 2016

@jfischer-phytec-iot could you tests this one and adapt changes into #5469?

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Sep 16, 2016 •
edited
Loading

PING!

(Edit) I'll squash in the meantime and let Murdock go over it

@smlng
kw2xrf: set tx_power in gnrc_netdev_t
fc9e1d9 
without this patch, tx_power is directly set on the device but not in
gnrc_netdev_t. Thus, calling ifconfig in shell shows tx_power always
at 0dBm, never showing the correct, current value. Additionally, it
verifies that given tx_power to be set is in valid range.
@smlng smlng force-pushed the pr/drivers/kw2xrf/set_tx_power branch from 1e0cc18 to fc9e1d9 Compare September 16, 2016 08:54
@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Sep 16, 2016
@smlng smlng added this to the Release 2016.10 milestone Sep 16, 2016
@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 16, 2016
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Sep 16, 2016

juhu, Murdock says okay--no weird, unrelated errors anymore--finally!

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Sep 21, 2016 •
edited
Loading

Murdock is happy, but after @OlegHahm enabled this review feature this requires an approved review. @jfischer-phytec-iot seems to be on vacation, anybody else ( @miri64, @cgundogan, @jremmert-phytec-iot ) willing to look over this? So we can move on ...

@miri64
Copy link
Copy Markdown
Member

miri64 commented Sep 21, 2016

There wasn't any ACK, right? So even with the old system this wouldn't have been mergeable ;-)

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Sep 21, 2016

@miri64 I hoped @jfischer-phytec-iot would ACK, but no response so far.

@jfischer-no
Copy link
Copy Markdown
Contributor

jfischer-no commented Sep 22, 2016

sorry, I am all the time busy with work and the usb stack, also I got no notifications from the github 😠

@jfischer-no jfischer-no merged commit 4a3ed29 into RIOT-OS:master Sep 22, 2016
@smlng smlng mentioned this pull request Sep 22, 2016
Closed
@smlng smlng deleted the pr/drivers/kw2xrf/set_tx_power branch September 27, 2016 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jfischer-no jfischer-no jfischer-no approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jfischer-no jfischer-no

Labels

Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2016.10

Development

Successfully merging this pull request may close these issues.

4 participants

@smlng @miri64 @cgundogan @jfischer-no