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

ieee802154: centralize default values #5977

Merged

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Oct 21, 2016

Fixes #3291.

I also changed up the selection of the default channel a little bit, since it was limited to 2.4 GHz IEEE 802.15.4 only. This solution isn't perfect either, but better than the previous one I think.

@miri64 miri64 added Area: network Area: Networking Area: drivers Area: Device drivers labels Oct 21, 2016
@miri64 miri64 added this to the Release 2016.10 milestone Oct 21, 2016
@miri64 miri64 force-pushed the ieee802154/enh/centralize-default-values branch from f686ab1 to 6d8d6fc Compare October 21, 2016 11:49
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

Besides the arguable naming issues, I tested for CC2538: works!

@@ -45,7 +45,7 @@ extern "C" {
/**
* @brief Maximum possible packet size in byte
*/
#define AT86RF2XX_MAX_PKT_LENGTH (127)
#define AT86RF2XX_MAX_PKT_LENGTH (IEEE802154_MAX_FRAME_LEN)
Copy link
Member

Choose a reason for hiding this comment

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

mhm, the naming seems somewhat inconsistent here it is IEEE802154_MAX_FRAME_LEN instead of IEEE802154_FRAME_LEN_MAX compare to above IEEE802154_CHANNEL_MAX. Or what's the rational?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is true. Will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed.

#endif
#define AT86RF2XX_MIN_CHANNEL (IEEE802154_CHANNEL_MIN)
#define AT86RF2XX_MAX_CHANNEL (IEEE802154_CHANNEL_MAX)
#define AT86RF2XX_DEFAULT_CHANNEL (IEEE802154_DEFAULT_CHANNEL)
Copy link
Member

Choose a reason for hiding this comment

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

same here why its not IEEE802154_CHANNEL_DEFAULT?

Copy link
Member Author

Choose a reason for hiding this comment

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

look at how they are grouped in net/ieee802154.h and this naming makes more sense ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I see these macros as legacy wrapper so if the definitions of those look a little bit weird I'm not that sad as a clean definition of the new macros ;-)


/**
* @brief Default TX power (0dBm)
*/
#define AT86RF2XX_DEFAULT_TXPOWER (0U)
#define AT86RF2XX_DEFAULT_TXPOWER (IEEE802154_DEFAULT_TXPOWER)
Copy link
Member

Choose a reason for hiding this comment

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

Dito here

Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

Looks good to me. Ack'n'Go after Murdock - and squash of last commit.

@smlng smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 22, 2016
@miri64 miri64 force-pushed the ieee802154/enh/centralize-default-values branch from 88d0016 to 4e789ed Compare October 22, 2016 12:42
@miri64
Copy link
Member Author

miri64 commented Oct 22, 2016

Squashed

@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 Oct 26, 2016
The config assumed it was a 2.4 GHz radio. This solution isn't perfect
either but eliminates a few more cases.
@miri64 miri64 force-pushed the ieee802154/enh/centralize-default-values branch from 4e789ed to 964cd75 Compare October 26, 2016 13:33
@miri64
Copy link
Member Author

miri64 commented Oct 26, 2016

Rebased to current master.

@miri64 miri64 merged commit 9e97b0a into RIOT-OS:master Oct 26, 2016
@miri64 miri64 deleted the ieee802154/enh/centralize-default-values branch October 26, 2016 14:39
@miri64 miri64 mentioned this pull request Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants