Conversation
Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>
…e I/O Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>
Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>
…platform Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>
Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>
…tests Signed-off-by: Adelin Dobre <adelin.dobre@rinftech.com>
alext-mkrs
left a comment
alext-mkrs
left a comment
There was a problem hiding this comment.
The concept looks ok in general, but please see/address those specific line comments.
| 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 + \ |
There was a problem hiding this comment.
Let's make this a base for MRAA_MOCK_PINCOUNT to avoid repeating the same piece two times.
| #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 + \ |
There was a problem hiding this comment.
style: shis and line below need one more space to align on opening (
| 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; |
There was a problem hiding this comment.
nit: as we don't expect this do be a negative please use uint or somesuch
| 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++) { |
There was a problem hiding this comment.
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.
| mraa_result_t | ||
| mraa_mock_pwm_init_raw_replace(mraa_pwm_context dev, int pin) | ||
| { | ||
| if(dev->pin != pin) { |
There was a problem hiding this comment.
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.
| int | ||
| mraa_mock_pwm_read_duty_replace(mraa_pwm_context dev) | ||
| { | ||
| if(dev->duty_fp > INT_MAX || dev->duty_fp < INT_MIN) { |
There was a problem hiding this comment.
While not strictly necessary, I'd suggest to put this/same check into the write_duty_replace as well.
| return NULL; | ||
| } | ||
|
|
||
| #if defined(MOCKPLAT) |
There was a problem hiding this comment.
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?
| #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 |
There was a problem hiding this comment.
- Why this change?
- If this is really necessary, you have to update the mock platform description in
docsas well. - If this is really necessary, the respective test updates must be in the same commit, so that
masteris never broken.
| #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 |
There was a problem hiding this comment.
Please update the mock platform description in docs accordingly, right now the PWM functionality and the extra pin are not mentioned.
| /* 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()); |
There was a problem hiding this comment.
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.
3 participants
No description provided.