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.

Sign up for GitHub

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

Jump to bottom

kw2xrf: set tx_power in gnrc_netdev_t #5834

Merged
merged 1 commit into from
Sep 22, 2016
Merged

kw2xrf: set tx_power in gnrc_netdev_t #5834

merged 1 commit into from
Sep 22, 2016

Conversation

smlng
Copy link
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
Member

miri64 commented Sep 9, 2016

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

@miri64
Copy link
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
Member

the same is missing for the at86rf driver

@miri64
Copy link
Member

miri64 commented Sep 9, 2016

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

@cgundogan
Copy link
Member

@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
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
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
Member

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
Member

cgundogan commented Sep 9, 2016 •
edited
Loading

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

@miri64
Copy link
Member

miri64 commented Sep 9, 2016

These changes should also be noted for #5469.

@smlng
Copy link
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
Member Author

smlng commented Sep 9, 2016 •
edited
Loading

and again: verified by live testing on hardware

(Edit): works!

@smlng
Copy link
Member Author

smlng commented Sep 9, 2016

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

@smlng
Copy link
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
Member Author

smlng commented Sep 16, 2016

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

@smlng
Copy link
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
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
Member Author

smlng commented Sep 21, 2016

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

@jfischer-no
Copy link
Contributor

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

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

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