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

at86rf2xx: different types have RSSI base values #6369

Merged
merged 1 commit into from
Jan 19, 2017
Merged

at86rf2xx: different types have RSSI base values #6369

merged 1 commit into from
Jan 19, 2017

Conversation

OlegHahm
Copy link
Member

Fixes #4874.

@OlegHahm OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers Area: network Area: Networking labels Jan 15, 2017
@OlegHahm OlegHahm added this to the Release 2017.01 milestone Jan 15, 2017
jnohlgard
jnohlgard requested changes Jan 16, 2017
View reviewed changes
Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

Verified the values for rf233 and rf231 via data sheet. RF212B needs some adjustments according to data sheet (table 8-16 RSSI_BASE_VAL).

drivers/include/at86rf2xx.h
#if MODULE_AT86RF233
# define RSSI_BASE_VAL (-94)
#elif MODULE_AT86RF212B
# define RSSI_BASE_VAL (-97)
Copy link
Member

Choose a reason for hiding this comment

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

it seems like this value depends on the PHY mode, according to the data sheet for 212B.
The mode which yields -97 is not settable directly via ifconfig x set page y. IMO it's better to default to -98, which is what the modes for page 2 give on both EU and US channel bands.
Also, add a note that the 212B value is valid for page 2 only. The value is -100 or -99 for page 0, depending on channel setting.

@kb2ma
Copy link
Member

kb2ma commented Jan 16, 2017

Overall, the use of these separate values looks good enough to me. I can't comment on the 212B personally, but just to add a data point, let me ask someone I think is using this chip.

@OlegHahm
Copy link
Member Author

Addressed your comments.

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jan 16, 2017
@kb2ma
Copy link
Member

kb2ma commented Jan 16, 2017

Sorry, no further information. I defer on the 212B.

PeterKietzmann
PeterKietzmann reviewed Jan 18, 2017
View reviewed changes
drivers/include/at86rf2xx.h
* @note for the default settings in RIOT fur the at86rf212b,
* for other seetings this value may change.
*/
# define RSSI_BASE_VAL (-98)
Copy link
Member

Choose a reason for hiding this comment

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

typo "fur" and unintended tap I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed typo, but which tap are you referring to?

Copy link
Member

Choose a reason for hiding this comment

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

Wasn't a tap but spaces between "'#" and "define".

@PeterKietzmann
Copy link
Member

Whoops, what hast the "twofish" stuff to do with this PR?

@OlegHahm OlegHahm force-pushed the at86rf2xx_rssi_base_value branch from c491bad to 6ad8c5c Compare January 18, 2017 21:44
@OlegHahm
Copy link
Member Author

Good question - rebase fixed it.

@PeterKietzmann
Copy link
Member

@OlegHahm previously I was talking about the spaces between "'#" and "define". Besides of that please squash

@OlegHahm OlegHahm force-pushed the at86rf2xx_rssi_base_value branch from 6ad8c5c to c04c5cb Compare January 18, 2017 22:20
@OlegHahm
Copy link
Member Author

The indentation is on purpose because of the if-else. Squashed.

@PeterKietzmann PeterKietzmann 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 Jan 18, 2017
@PeterKietzmann
Copy link
Member

Well, this indentation style for macros is new to me but I I won't stall this PR because of that.

@PeterKietzmann
Copy link
Member

Ah, can't merge until @gebart approves.

@OlegHahm
Copy link
Member Author

Well, this indentation style for macros is new to me

@PeterKietzmann, we use it rather frequently in RIOT. The reason is that the standard requires that the first character of the line for a preprocessor directive is the hash character.

@PeterKietzmann PeterKietzmann merged commit b92ce8a into RIOT-OS:master Jan 19, 2017
@OlegHahm OlegHahm deleted the at86rf2xx_rssi_base_value branch January 19, 2017 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnohlgard jnohlgard jnohlgard approved these changes

@PeterKietzmann PeterKietzmann PeterKietzmann approved these changes

Assignees

@jnohlgard jnohlgard

@kb2ma kb2ma

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
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@OlegHahm @kb2ma @PeterKietzmann @jnohlgard