Mock platform #949

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

Jump to bottom

Open

stefan-andritoiu wants to merge 6 commits into eclipse:master

from stefan-andritoiu:mock_platform

Open

Mock platform #949

stefan-andritoiu wants to merge 6 commits into eclipse:master from stefan-andritoiu:mock_platform

+317 −126

Conversation

@stefan-andritoiu

Copy link

Contributor

@stefan-andritoiu stefan-andritoiu commented Jan 28, 2019

No description provided.

adelindobre added 6 commits

November 7, 2018 17:06

@adelindobre


 initio: Add safety checks inside MraaIo destructor

54e9fc7

Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>

@adelindobre


 mock_platform: Mock functionalities for pwm / Add support for multipl...

d383c4c

...e I/O
Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>

@adelindobre


 api_common_hpp_unit: Change getPinCount test for mock_platform

d3f7e65

Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>

@adelindobre


 platform_checks.py: Change PLATFORM_PINCOUNT after pwm added on mock_...

15b4384

...platform
Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>

@adelindobre


 mock_board_i2c: Increase register block length

b7e348c

Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>

@adelindobre


 tests/mock/i2c_checks: Change MRAA_MOCK_I2C_DATA_LEN and correct two ...

b116331

...tests
Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>

alext-mkrs

alext-mkrs suggested changes

Feb 3, 2019

View reviewed changes

Copy link

Contributor

@alext-mkrs alext-mkrs left a comment

Choose a reason for hiding this comment

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

The concept looks ok in general, but please see/address those specific line comments.

include/mock/mock_board.h

2 * MRAA_MOCK_I2C_BUS_COUNT + 4 * MRAA_MOCK_SPI_BUS_COUNT + \

MRAA_MOCK_PWM_DEV_COUNT + 2 * MRAA_MOCK_UART_DEV_COUNT)

#define MRAA_PWM_OFFSET (MRAA_MOCK_GPIO_COUNT + MRAA_MOCK_AIO_COUNT + \

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

Let's make this a base for MRAA_MOCK_PINCOUNT to avoid repeating the same piece two times.

include/mock/mock_board.h

#define MRAA_MOCK_PWM_DEV_COUNT 1

#define MRAA_MOCK_UART_DEV_COUNT 1

#define MRAA_MOCK_PINCOUNT (MRAA_MOCK_GPIO_COUNT + MRAA_MOCK_AIO_COUNT + \

2 * MRAA_MOCK_I2C_BUS_COUNT + 4 * MRAA_MOCK_SPI_BUS_COUNT + \

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

style: shis and line below need one more space to align on opening (

src/mock/mock_board.c

b->i2c_bus[0].bus_id = 0;

b->i2c_bus[0].sda = 2;

b->i2c_bus[0].scl = 3;

int i2c_offset = MRAA_MOCK_GPIO_COUNT + MRAA_MOCK_AIO_COUNT;

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

nit: as we don't expect this do be a negative please use uint or somesuch

src/mock/mock_board.c

int i2c_offset = MRAA_MOCK_GPIO_COUNT + MRAA_MOCK_AIO_COUNT;

b->i2c_bus_count = MRAA_MOCK_I2C_BUS_COUNT;

for(int index = 0; index < MRAA_MOCK_I2C_BUS_COUNT; index++) {

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

Please add a space after for and before ( to align with mraa code style. Better yet - please run clang-format on your submissions with the confir file from the repo root.

src/mock/mock_board_pwm.c

mraa_result_t

mraa_mock_pwm_init_raw_replace(mraa_pwm_context dev, int pin)

{

if(dev->pin != pin) {

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

Please add a space after if and before ( to align with mraa code style. Better yet - please run clang-format on your submissions with the confir file from the repo root.

src/mock/mock_board_pwm.c

int

mraa_mock_pwm_read_duty_replace(mraa_pwm_context dev)

{

if(dev->duty_fp > INT_MAX || dev->duty_fp < INT_MIN) {

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

While not strictly necessary, I'd suggest to put this/same check into the write_duty_replace as well.

src/pwm/pwm.c

return NULL;

}

#if defined(MOCKPLAT)

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

To be honest, I don't like changing base files for specific platforms, that's what replace functions are for. Why can't this be done there or why can't you pass the shifted value right away?

include/mock/mock_board_i2c.h

#define MOCK_I2C_DEV_ADDR 0x33

// Mock I2C device data registers block length in bytes. Our code assumes it's >= 1.

#define MOCK_I2C_DEV_DATA_LEN 10

#define MOCK_I2C_DEV_DATA_LEN 255

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

Why this change?
If this is really necessary, you have to update the mock platform description in docs as well.
If this is really necessary, the respective test updates must be in the same commit, so that master is never broken.

include/mock/mock_board.h

#define MRAA_MOCK_AIO_COUNT 1

#define MRAA_MOCK_I2C_BUS_COUNT 1

#define MRAA_MOCK_SPI_BUS_COUNT 1

#define MRAA_MOCK_PWM_DEV_COUNT 1

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

Please update the mock platform description in docs accordingly, right now the PWM functionality and the extra pin are not mentioned.

tests/unit/api/api_common_h_unit.cxx

/* Currently 10 pins in the mock platform */

ASSERT_EQ(10, mraa_get_pin_count());

/* Currently 11 pins in the mock platform */

ASSERT_EQ(11, mraa_get_pin_count());

Copy link

Contributor

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

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

Please combine all those test changes into the same commit that introduces the PWM pin - so that the pin addition goes in as an atomic change and we have both functionality and tests for it right away.

Labels

None yet

3 participants

@stefan-andritoiu @alext-mkrs @adelindobre

Mock platform #949

Are you sure you want to change the base?

Mock platform #949

Conversation

@stefan-andritoiu stefan-andritoiu commented Jan 28, 2019

Uh oh!

@alext-mkrs alext-mkrs left a comment

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

@alext-mkrs alext-mkrs Feb 3, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants