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

Add test for periph timer #30

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MichelRottleuthner
Copy link
Contributor

This the first few tests for periph/timer.

To make this work properly on all boards we need to provide the debug pin configuration with the environment, i.e. tell the test which pin of the DUT is connected to DEBUG0 of PHiLIP

Known issues:
The trace mechanism of PHiLIP sometimes reports timings that are not accurate.
This causes problems with the jitter test because sometimes delays around ~1.8 ms are added to the actual measurement. I verified with the scope that the timing deviation in this case actually didn't come from the DUT.
I discussed this offline with @MrKevinWeiss and we agreed that the best solution for this would be to use the capture features of the PHiLIP timer.

@MrKevinWeiss MrKevinWeiss self-assigned this Sep 2, 2019
@MrKevinWeiss MrKevinWeiss added the enhancement New feature or request label Sep 2, 2019
@MrKevinWeiss
Copy link
Collaborator

@MichelRottleuthner can you squash please?

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.

I think the python interface and robot tests can be structured and simplified, however that can be done in a follow up PR (by me). Still some changes to the shell-based test firmware

cmd_timer_bench_read },
{ "timer_init", "init_timer", cmd_timer_init },
{ "timer_set", "set timer to relative value", cmd_timer_set },
{ "timer_absolute", "set timer to absolute value", cmd_timer_absolute },
Copy link
Member

Choose a reason for hiding this comment

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

should be timer_set_absolute to match (current) API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

timer_start(timer_dev);
puts("Success: timer_start()\n");
Copy link
Member

Choose a reason for hiding this comment

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

better use _print_cmd_result here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

timer_stop(timer_dev);
puts("Success: timer_stop()\n");
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, use _print_cmd_result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


static gpio_t _get_pin(const char *port_str, const char *pin_str)
{
int port = _get_num(port_str);
Copy link
Member

Choose a reason for hiding this comment

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

error case is not checked here and below, i.e. CONVER_ERROR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


if (temp == str || *temp != '\0' ||
((val == LONG_MIN || val == LONG_MAX) && errno == ERANGE)) {
val = CONVERT_ERROR;
Copy link
Member

Choose a reason for hiding this comment

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

the error case is not evaluated in many cases where this function is used. Maybe change to

static inline int _get_num(const char *str, uint32_t *val)

and return value by reference and error as return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@smlng smlng mentioned this pull request Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants