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

tests/drivers/at: add check if device is initialized before sending command #20140

Merged
merged 2 commits into from
Apr 19, 2024
Merged

tests/drivers/at: add check if device is initialized before sending command #20140

merged 2 commits into from
Apr 19, 2024

Conversation

krzysztof-cabaj
Copy link
Contributor

Contribution description

This PR adds to the tests/driver/at send... functions checks if AT device is initialized and power on.

Testing procedure

Flash device with tests/driver/at program and try to send some AT command without initialization and power on.
Without this PR send... commands waits for timeout.
With this PR appropriate error message is shown.

Issues/PRs references

None.

@krzysztof-cabaj krzysztof-cabaj force-pushed the tests-drivers-at branch 3 times, most recently from 6a3ee00 to 619ee31 Compare December 6, 2023 19:22
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Dec 6, 2023
kfessel
kfessel reviewed Dec 7, 2023
View reviewed changes
tests/drivers/at/main.c Outdated
Comment on lines 439 to 440
initialized = false;
is_power_on = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about this PR

but for initialization you should just use the initializer instead of setting the value at the start of the program

l 36 and l 37

false (0) should also be the default initializer, in c static values are initialized (unless you tell the compiler it should not) -> not writing the initializer might also be OK

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 move initialization form main function to the static variable initializer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checking for initialization is fine, since using the at_* methods without proper initialization would be UB. I'm not sure about the power state tho. What if we want to test an error case, e.g. proper command timeout if the module is powered off?

@krzysztof-cabaj
Copy link
Contributor Author

@derMihai I could remove checks for power off or maybe add ability to switch on/off this check.

The reason for this PR is that when testing few clones of HM-10 in various configurations, few times I do not properly initialize/power on device - and searching for a longer while where is a problem ;).

@krzysztof-cabaj
Copy link
Contributor Author

Dear all,

I observe that from my last activity in this PR there are some changes in the code which lead to merge conflicts.
I rebase the PR code and fix all issues.

Could we move this PR little forward?

@derMihai
Copy link
Contributor

derMihai commented Apr 16, 2024 •
edited
Loading

Hi,
like I said, early exits on un-initialized AT driver are a good thing, but not on a powered off device. Running the test functions on a powered off device is a valid test case. You may leave the warnings in, but please don't change the control flow.

On a second thought, the at_dev_power*() functions have more to do with the serial device than the modem. So I guess is valid to check that. This PR is fine the way it is from me.

@krzysztof-cabaj
Copy link
Contributor Author

@derMihai so should I leave this PR as is?

@derMihai
Copy link
Contributor

from my side, yes.

kfessel
kfessel approved these changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

simplify the test usage -someone testing their setup,
special tests (timeout behavior) might be added later.

@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 16, 2024
@riot-ci
Copy link

riot-ci commented Apr 16, 2024

Murdock results

✔️ PASSED

59d5af7 tests/drivers/at: add static variable initializer

Success Failures Total Runtime
176 0 176 01m:15s

Artifacts

@krzysztof-cabaj
Copy link
Contributor Author

The merge queue is empty ... maybe we can use this ;)

@kfessel kfessel added this pull request to the merge queue Apr 19, 2024
Merged via the queue into RIOT-OS:master with commit f130ebf Apr 19, 2024
27 checks passed
@mguetschow mguetschow added this to the Release 2024.07 milestone Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derMihai derMihai derMihai left review comments

@kfessel kfessel kfessel approved these changes

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

Assignees
No one assigned
Labels
Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2024.07
Development

Successfully merging this pull request may close these issues.
5 participants
@krzysztof-cabaj @derMihai @riot-ci @kfessel @mguetschow