You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2019/12/11 08:34:49 UTC
[GitHub] [mynewt-core] brianwyld commented on issue #2106: PR for 3 changes
: NDEBUG build, fix SPI on STM32, bitbang uart tx only
brianwyld commented on issue #2106: PR for 3 changes : NDEBUG build, fix SPI on STM32, bitbang uart tx only
URL: https://github.com/apache/mynewt-core/pull/2106#issuecomment-564435150
Hi,
Thanks for your comments. I will update the code as per and attempt as 3
separate PRs.
To address specifc points:
- compilation with NDEBUG : even if it is mynewt 'policy' to have
assert() enabled generally, the code should at least compile when a
standard define is set and currently mynewt does not. The problem is that
currently it does not compile without warnings.
- the hal_uart.c file in STM32 code is indeed missing the include file (I
agree this is strange but there you go...)
thanks
Brian
On Tue, 10 Dec 2019 at 22:44, kasjer <no...@github.com> wrote:
> *@kasjer* commented on this pull request.
>
> In such form there is slim chance that anyone will accept this PR.
>
> There is chance that bitbang uart TX stuff is acceptable (as separate PR)
>
> Current assert failure on most platforms in mynewt leads to system panic.
> To be aligned to ANSI standard, all asserts found in mynewt code should
> changed to something mynewt specific like ASSERT_PANIC(). Then it would be
> clear what will happen if condition is false and there would not be space
> for ambiguity of assert() + NDEBUG.
> This could be brought to mailing list.
> ------------------------------
>
> In hw/bus/src/bus.c
> <https://github.com/apache/mynewt-core/pull/2106#discussion_r356278518>:
>
> > @@ -530,9 +530,12 @@ bus_node_unlock(struct os_dev *node)
> * structs are broken) and OS_BAD_MUTEX (unlock shall be only done by the
> * same task which locked it).
> */
> - assert(err == OS_OK || err == OS_NOT_STARTED);
> -
> - return 0;
> + if (err == OS_OK || err == OS_NOT_STARTED) {
> + return 0;
> + }
> + // Anything else is assertable (or if no asserts, return error)
>
> C++ style comments are not used in mynewt
> ------------------------------
>
> In hw/bus/src/bus.c
> <https://github.com/apache/mynewt-core/pull/2106#discussion_r356279147>:
>
> > @@ -530,9 +530,12 @@ bus_node_unlock(struct os_dev *node)
> * structs are broken) and OS_BAD_MUTEX (unlock shall be only done by the
> * same task which locked it).
> */
> - assert(err == OS_OK || err == OS_NOT_STARTED);
> -
> - return 0;
> + if (err == OS_OK || err == OS_NOT_STARTED) {
> + return 0;
> + }
> + // Anything else is assertable (or if no asserts, return error)
> + assert(0);
> + return -1;
>
> At this point rc has some error code (other then 0) why change something
> that may be meaningful to -1 that maybe less informative
> ------------------------------
>
> In hw/drivers/uart/uart_bitbang/pkg.yml
> <https://github.com/apache/mynewt-core/pull/2106#discussion_r356279742>:
>
> > @@ -28,4 +28,4 @@ pkg.deps:
> - "@apache-mynewt-core/hw/drivers/uart"
>
> pkg.init:
> - uart_bitbang_pkg_init: 'MYNEWT_VAL(UARTBB_SYSINIT_STAGE)'
> +# uart_bitbang_pkg_init: 'MYNEWT_VAL(UARTBB_SYSINIT_STAGE)'
>
> if something is removed it should be gone and not commented out.
> ------------------------------
>
> In hw/hal/src/hal_flash.c
> <https://github.com/apache/mynewt-core/pull/2106#discussion_r356279859>:
>
> > @@ -245,7 +245,7 @@ hal_flash_erase(uint8_t id, uint32_t address, uint32_t num_bytes)
> uint32_t end;
> uint32_t end_area;
> int i;
> - int rc;
> + int rc=0;
>
> missing spaces around =
> ------------------------------
>
> In hw/mcu/stm/stm32_common/src/hal_spi.c
> <https://github.com/apache/mynewt-core/pull/2106#discussion_r356281677>:
>
> > @@ -172,6 +172,7 @@ stm32_resolve_spi_irq(SPI_HandleTypeDef *hspi)
> #endif
> default:
> assert(0);
> + return 0;
>
> returning 0 here, would lead to unpredictable behavior.
> ------------------------------
>
> In hw/mcu/stm/stm32_common/src/hal_uart.c
> <https://github.com/apache/mynewt-core/pull/2106#discussion_r356282220>:
>
> > @@ -24,6 +24,7 @@
> #include "bsp/bsp.h"
> #include <assert.h>
> #include <stdlib.h>
> +#include "os/mynewt.h"
>
> It seems strange that this file missed include file (please confirmed)
> ------------------------------
>
> In
> hw/mcu/stm/stm32l1xx/src/ext/Drivers/STM32L1xx_HAL_Driver/Src/stm32l1xx_hal_spi.c
> <https://github.com/apache/mynewt-core/pull/2106#discussion_r356282666>:
>
> > @@ -170,7 +170,8 @@ static HAL_StatusTypeDef SPI_WaitOnFlagUntilTimeout(struct __SPI_HandleTypeDef *
> @endverbatim
> * @{
> */
> -
> +#if 0
> +// BW : The weak direction is ignored by gcc, and so fails to pick up the proper implementation from stm32l1xx_hal_spi_ex.c
>
> C++ style comment should not be used.
> Adding *BW :* is out of place
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/mynewt-core/pull/2106?email_source=notifications&email_token=AMIHRICFZDLHZBI4WCSU2WTQYAEVTA5CNFSM4JQA2ZYKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOW2EMI#pullrequestreview-330146353>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AMIHRIDYM2JZU727OLIDJHDQYAEVTANCNFSM4JQA2ZYA>
> .
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services