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

Xbee fixes #6406

Merged
merged 5 commits into from
Jan 20, 2017
Merged

Xbee fixes #6406

merged 5 commits into from
Jan 20, 2017

Conversation

kYc0o
Copy link
Contributor

@kYc0o kYc0o commented Jan 18, 2017

Some XBee fixes, especially to work with small platforms (e.g. AVR8)

@kYc0o kYc0o added Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking labels Jan 18, 2017
@kYc0o kYc0o added this to the Release 2017.01 milestone Jan 18, 2017
OlegHahm
OlegHahm requested changes Jan 18, 2017
View reviewed changes
examples/gnrc_minimal/Makefile
@@ -13,7 +13,7 @@ BOARD_INSUFFICIENT_MEMORY := chronos msb-430 msb-430h
#USEMODULE += xbee

## set default UART to use in case none was defined
#XBEE_UART ?= "UART_NUMOF-1"
#XBEE_UART ?= "1"
Copy link
Member

Choose a reason for hiding this comment

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

What's the idea of this change? The current version seems to be more flexible to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I find that the most common usage of XBee is on platforms which offer more than one UART, especially for Arduino boards, on which the XBee shield is attached to UART0.

Using the current approach the XBee goes to UART3 in arduino-mega2560, which is not evident to know IMHO. Additionally, on platforms with only 1 UART it goes to UART0, which is usually used for STDIO, thus conflicting since we don't have any check in the current master to avoid such conflict.

Thus, I prefer to state exactly which UART I'm going to use, and if it doesn't exists at least I'll have STDIO free.

Copy link
Contributor

Choose a reason for hiding this comment

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

either way, this should be XBEE_UART ?= UART_DEV\(x\)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not done in line 21?

Copy link
Member

Choose a reason for hiding this comment

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

What happens on Boards where UART_NUMOF == 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I mean #XBEE_UART ?= "UART_NUMOF-1" and #XBEE_UART ?= "1"

Copy link
Member

Choose a reason for hiding this comment

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

But I thought this is wrong anyway?

Copy link
Member

Choose a reason for hiding this comment

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

And what is the assertion about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what's wrong or not, I just want to have a clear definition of which UART is being used by the XBee device, for me it's somehow ambiguous this UART_NUMOF-1, since anyways I'm quite sure to which UART I'm connecting my device.

For the assertion is in this line. Just checks if the UART_NUMOF is greater than the UART device used by the XBee.

Copy link
Contributor

Choose a reason for hiding this comment

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

forget my comment above...

haukepetersen
haukepetersen requested changes Jan 18, 2017
View reviewed changes
drivers/xbee/xbee.c
@@ -41,11 +41,11 @@
/**
* @brief Delay when entering command mode, must be > 1s
*/
#define ENTER_CMD_MODE_DELAY (1100U * 1000U)
#define ENTER_CMD_MODE_DELAY (1100UL * SEC_IN_MS)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you mean MS_IN_USEC here?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops... yes, you're right, will change.

examples/gnrc_minimal/Makefile
@@ -13,7 +13,7 @@ BOARD_INSUFFICIENT_MEMORY := chronos msb-430 msb-430h
#USEMODULE += xbee

## set default UART to use in case none was defined
#XBEE_UART ?= "UART_NUMOF-1"
#XBEE_UART ?= "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

either way, this should be XBEE_UART ?= UART_DEV\(x\)...

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 18, 2017

Can I squash and let Murdock agree?

haukepetersen
haukepetersen approved these changes Jan 18, 2017
View reviewed changes
Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

please squash

@kYc0o kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 18, 2017
@haukepetersen
Copy link
Contributor

@OlegHahm are you happy, too?

@OlegHahm
Copy link
Member

Is it on purpose to activate DEVELHELP for the test?

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 19, 2017

Yes, it ensures if the XBee is correctly configured on a valid UART port.

@OlegHahm
Copy link
Member

How so?

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 19, 2017

With the assertion I talked about above.

@OlegHahm
Copy link
Member

The assertions just checks if the UART exist, right? So, you can still misconfigure your XBEE. I don't have a strong opinion, just found it noteworthy that most other tests don't use DEVELHELP to simulate a production-like environment.

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 19, 2017

No, the assertion checks if the UART is not UART0 and if it's not higher than the number of available UARTs.

I must use this solution since we don't have any other way to check misconfiguration in production-like environment.

Maybe #6408 wold help.

@OlegHahm
Copy link
Member

No, the assertion checks if the UART is not UART0 and if it's not higher than the number of available UARTs.

Yes, that's what I meant.

I must use this solution since we don't have any other way to check misconfiguration in production-like environment.

I don't get this. In a production environment, you would not use DEVELHELP and assertions. If you misconfigure (connect the XBEE module to UART 3, but configure UART 2), you're doomed anyway.

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 19, 2017

Yes I agree with you, I only want a way to make clear what the test fails, using the former approach it just fails without any apparent reason.

I let you the honour to merge it.

@kYc0o
Copy link
Contributor Author

kYc0o commented Jan 20, 2017

Rebased to include #6399 .

@PeterKietzmann
Copy link
Member

@OlegHahm CI has passed. Mind to take a final look at the changes and merge in best case?

@OlegHahm OlegHahm merged commit 4239075 into RIOT-OS:master Jan 20, 2017
@OlegHahm
Copy link
Member

Sure :)

@kYc0o kYc0o deleted the xbee_fixes branch January 20, 2017 15:42
@kYc0o kYc0o mentioned this pull request Jan 20, 2017
Closed
@miri64 miri64 mentioned this pull request Oct 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OlegHahm OlegHahm OlegHahm approved these changes

@haukepetersen haukepetersen haukepetersen approved these changes

Assignees

@aabadie aabadie

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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kYc0o @haukepetersen @OlegHahm @PeterKietzmann @aabadie