added spi_get_baudrate() + some consistency changes #395

Reneg973 · 2021-05-11T10:08:02Z

new function to query the current baudrate
new helper check whether SPI is busy

consistency changes

return a boolean for functions with a "is; has; can" in name
return unsigned value for number of bytes read/written
some const correctness

- new function to query the current baudrate + consistency changes - return a boolean for functions with a "is; has; can" in name - return unsigned value for number of bytes read/written - some const correctness

lurch · 2021-05-11T10:48:33Z

GitHub says that this has a conflict already?

src/rp2_common/hardware_spi/include/hardware/spi.h

src/rp2_common/hardware_spi/spi.c

src/rp2_common/hardware_spi/include/hardware/spi.h

kilograham · 2021-05-11T12:06:49Z

src/rp2_common/hardware_spi/include/hardware/spi.h

    invalid_params_if(SPI, spi != spi0 && spi != spi1);
    return spi == spi1 ? 1 : 0;
 }

-static inline spi_hw_t *spi_get_hw(spi_inst_t *spi) {
+static inline spi_hw_t *spi_get_hw(const spi_inst_t *spi) {


don't think this should be const; sure the method itself doesn't write thru it, but it sure does cast it to a mutable pointer

I propose an alternative, add an additional function:
static inline const spi_hw_t *spi_get_hw_const(const spi_inst_t *spi) {

Needs some more changes. Agreed?

we don't do this for any hardware; not least there may be registers for which reading has side effects.

Your decision, both ways are possible. Anyway my experience is that const correctness can expose possible issues in code.

E.g. check function hw_is_claimed(), the function name says it just checks, but in reality it claims, so it does more than expected. One of the rules I try to follow - no surprises. And if hw_is_claimed() modifies something another function (hw_claim_or_assert) is made for, IMO this is a surprise

ha - yes that is a bug! I'm all for const pointers to any data struct, i was just on the fence in the h/w pointer case. Anyway, we can't do this as it requires a cast away of const... as you say we could add a spi_get_hw_const which I'm not whoilly against, but I don't think is necessary.

right, that's ok here, it's just a simple function where const correctness is not of any meaning. I'll remove it - but then it must be removed for every const correct function which uses it... compiler thingy

I made a proposal with commit 5a043a2. Please check whether it's ok or needs a revert

src/rp2_common/hardware_spi/include/hardware/spi.h

removed misleading docu

…to SPI-Helpers

src/rp2_common/hardware_spi/include/hardware/spi.h

kilograham

please rebase onto latest develop (as there is a merge conflict)

kilograham · 2021-05-13T12:45:42Z

src/rp2_common/hardware_spi/include/hardware/spi.h

    invalid_params_if(SPI, spi != spi0 && spi != spi1);
    return spi == spi1 ? 1 : 0;
 }

-static inline spi_hw_t *spi_get_hw(spi_inst_t *spi) {
+static inline spi_hw_t *spi_get_hw(const spi_inst_t *spi) {


ha - yes that is a bug! I'm all for const pointers to any data struct, i was just on the fence in the h/w pointer case. Anyway, we can't do this as it requires a cast away of const... as you say we could add a spi_get_hw_const which I'm not whoilly against, but I don't think is necessary.

kilograham

try building kitchen_sink ... there are a bunch of compiler warnings

kilograham · 2021-05-13T20:36:51Z

Also, frankly I'm not convinced by switching the return values to size_t... generally we haven't been using size_t stylistically (I know that the params were size_t), however where functions do return a value it tends to have the possibility of an error code.

Reneg973 · 2021-05-14T08:35:21Z

try building kitchen_sink ... there are a bunch of compiler warnings

thanks for the tip. Building this also finds other warnings like unused functions in stdio and a missing delete operator in new_delete.cpp. How do you handle these?

Reneg973 · 2021-05-14T08:46:44Z

however where functions do return a value it tends to have the possibility of an error code.

In general this is fine. Anyway for these functions currently there are 3 arguments against it.

the function never returns an error (it is automagically included, if the return is != input len)
the documentation doesn't tell anything about possible error code
even if the use case may not exist, providing a size_t len which is larger than int will result in a negative value, which is not expected.

Checking for transfer error is still possible by comparing the return value with the input len, but now without a signed/unsigned mismatch comparison.

lurch · 2021-05-14T09:19:03Z

a missing delete operator in new_delete.cpp

See #304 ?

kilograham · 2021-05-14T10:43:54Z

try building kitchen_sink ... there are a bunch of compiler warnings

thanks for the tip. Building this also finds other warnings like unused functions in stdio and a missing delete operator in new_delete.cpp. How do you handle these?

hmm; what compiler are you using... i don't see those warnings

kilograham · 2021-05-14T10:46:48Z

however where functions do return a value it tends to have the possibility of an error code.

In general this is fine. Anyway for these functions currently there are 3 arguments against it.
1. the function never returns an error (it is automagically included, if the return is != input len)

2. the documentation doesn't tell anything about possible error code

3. even if the use case may not exist, providing a size_t len which is larger than int will result in a negative value, which is not expected.
Checking for transfer error is still possible by comparing the return value with the input len, but now without a signed/unsigned mismatch comparison.

Yes, i was talking about consistency in the API with other APIs, so this is in general.

yes, but doesn't give you the option of return what error
sure
this is a 32 bit microcontroller; providing a >31 bit value is never going to be valid... this is why size_t wasn't used anywhere really (and when I say anywhere, yes these APIs do use it).

Reneg973 · 2021-05-14T11:13:25Z

hmm; what compiler are you using... i don't see those warnings

PICO_GCC_TRIPLE defaulted to arm-none-eabi
-- The C compiler identification is GNU 10.2.1
-- The CXX compiler identification is GNU 10.2.1

[57/259] Building C object CMakeFiles/kitchen_sink_no_flash.dir/D_/PicoSDK/1.1.2-Git/src/rp2_common/pico_unique_id/unique_id.c.obj
[58/259] Building C object CMakeFiles/kitchen_sink_no_flash.dir/D_/PicoSDK/1.1.2-Git/src/rp2_common/pico_multicore/multicore.c.obj
[59/259] Building C object CMakeFiles/kitchen_sink_no_flash.dir/D_/PicoSDK/1.1.2-Git/src/rp2_common/pico_stdio/stdio.c.obj
FAILED: CMakeFiles/kitchen_sink_no_flash.dir/D_/PicoSDK/1.1.2-Git/src/rp2_common/pico_stdio/stdio.c.obj
D:\2020-q4-major\bin\arm-none-eabi-gcc.exe -DPARAM_ASSERTIONS_ENABLE_ALL=1 -DPICO_AUDIO_DMA_IRQ=1 -DPICO_BIT_OPS_PICO=1 ... lots_of_includes... pico_stdio/stdio.c
D:/PicoSDK/1.1.2-Git/src/rp2_common/pico_stdio/stdio.c:203:13: error: 'stdio_buffered_printer' defined but not used [-Werror=unused-function]

Reneg973 · 2021-05-14T11:48:51Z

Yes, i was talking about consistency in the API with other APIs, so this is in general.

Ok, so revert?

kilograham · 2021-05-14T15:05:35Z

Yes, i was talking about consistency in the API with other APIs, so this is in general.

Ok, so revert?

yes please, whilst i can see why you might want to, I don't see enough value in changing the API

lurch · 2021-05-14T15:21:28Z

D:/PicoSDK/1.1.2-Git/src/rp2_common/pico_stdio/stdio.c:203:13: error: 'stdio_buffered_printer' defined but not used [-Werror=unused-function]

Perhaps that function just needs a #if LIB_PICO_PRINTF_PICO wrapped around it? https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/pico_stdio/stdio.c#L219

Reneg973 · 2021-05-14T17:02:46Z

ok is reverted. Did you realize that these function in no case return the transferred len? Should I do this with this PR too?

kilograham · 2021-05-14T17:18:23Z

ok is reverted. Did you realize that these function in no case return the transferred len? Should I do this with this PR too?

do you mean in all cases?

Reneg973 · 2021-05-14T17:30:04Z

ok is reverted. Did you realize that these function in no case return the transferred len? Should I do this with this PR too?

do you mean in all cases?

I mean they return the input length and not what was really sent => wrong docu or wrong implementation?

lurch · 2021-05-14T18:12:46Z

I mean they return the input length and not what was really sent

This may be a stupid question, but for SPI how would those numbers ever be different? 😕

Reneg973 · 2021-05-14T18:52:00Z

I mean they return the input length and not what was really sent

This may be a stupid question, but for SPI how would those numbers ever be different? 😕

Not with SPI itself, of course. But I am sure you heard about bit failures :)

Ok, it just a bit confusing for me seeing interfaces which force the caller to do error checking but there is no error checking in implementation at all.

See release notes for more descriptive details * Delete some redundant CMake parts (#240) * pio: Add 'pragma once' to generated header files (#237) * pio: allow programs with 32 instructions (#236) * Fixup incorrect doxygen for multicore_fifo_wready * Add param-validation to spin_lock_instance * Fix back-to-front description of IRQ priority in doxygen (#245) * Fix ROSC typo (#259) * Typo (#251) * Add gpio_get_out_level() accessor, and correct SIO GPIO_OUT struct ty… (#247) * Add gpio_get_out_level() accessor, and correct SIO GPIO_OUT struct type from WO to RW * Add pico_get_unique_board_id_string API (#281) * Clean up -Wconversion=error issues * move PLL reset code from clocks driver to pll driver (#110) * Don't clear PLL PWR/FBDIV after reset as unnecessary. Call out in runtime.c why USB/syscfg aren't reset. * i2c: set hold time of SDA during transmit to an appropriate value (#273) * i2c: set hold time of SDA during transmit to 2 for TCS34725 color sensor * i2c: fix issues in i2c_write_blocking_internal * i2c: rename sda_hold_count to sda_tx_hold_count * use assert rather than invalid_params_if for internal consistency checks * i2c: use a more appropriate sda tx hold time at higher baudrates * i2c: reduce 120/1e9 to the smallest possible integer numerator and denominator * Update NULL GPIO function to 0x1f (#320) * i2c: set high and low times to values that conform to the i2c specification (#314) * Make flash_do_cmd public (#269) * Fix implementation config listing in structs/i2c.h (#324) * Clarify that cache is flushed, but that function is intended for low-level metadata access during startup (#322) * Fix implementation config listing in structs/i2c.h (#325) * Fix param-validation for PIO sideset encoding (#311) * Remove MASTER_ON_HOLD bit from I2C status registers. Fix typos. (#326) * Fixing arithmetic underflow in SPI I/O loops #337 (#338) * Source code licence clarification (#340) * Updated existing Pimoroni board headers to match latest style, and added a new board (#343) * Added new pimoroni board headers * SPI Definitions for SparkFun boards (#344) * SPI Definitions for SparkFun boards * Clarify multicore_fifo doxygen (#323) Based on my observations in #284 * correct adafruit flash size for itsybitsy and qt rp2040 (#348) from 4 MB to 8 MB * Small typos (#366) * make spi_init return baud rate set (#296) * Fix path + typo in README.md (#347) * Fix path + typo in README.md * Remove incorrect path change * Remove typo * disable core 0 SIO FIFO IRQ handler during core 1 launch in case someone has already installed one (#375) * add PICO_DIVIDER_DISABLE_INTERRUPTS flag which makes PICO_DIVIDER disable interrupts around division rather than using co-operative guards to protect nested use (i.e. within IRQ/exception). Use of this flag can simplify porting of RTOSes but with a different performance profile (#372) * make all non hardware_ libraries foo add C preprocessor definition LIB_FOO=1, and remove bespoke definitions which were all undocumented anyway (#374) * Change various (confusing to user) message to be DEBUG only (#365) * add small delay to stdio_get_until to prevent starvation of USB IRQ handler due to in use mutex. build was non deterministic due to missing link wrapping of getchar (#364) * Some cmake build improvements (#376) * Change some cmake output to DEBUG level Make SDK build more consistent with other libraries (use an INTERFACE marker library for inclusion tests) Add PICO_SDK_PRE_LIST_FILES, PICO_SDK_POST_LIST_FILES build vars * fix typo * remove leftover debugging message * i2c: improve communication with i2c devices in i2c_write_blocking * Definitions for IC_TX_BUFFER_DEPTH inconsistent (fixes #335) (#381) * Add hardware_exception for setting exception handlers at runtime (#380) * add __always_inline to trivial super low level inline functions (#379) * Rework lock_core / timers (#378) * remove spurious sys/select.h include (#377) * Fixup IRQ_PRIORITY #define values (#393) * Correct doxygen for mutex_try_enter (#392) * Fix a bunch of doxygen typos (#391) * Rework ordering of cmake, so that libraries in subdirectories can add to internal lists as PICO_SDK_POST_LIST_FILES, PICO_CONFIG_HEADER_FILES etc. (#382) * Fix some hardware_library dependencies (#383) * make host pico_platform.h and binary_info.h CMakeLists.txt safe for inclusion in non SDK build (#388) * Add basic CMSIS core headers (#384) * Fix the PICO_CONFIG default value for PICO_CMSIS_RENAME_EXCEPTIONS (#399) * add timeout_us/until to mutex/sem blocking methods (#402) * Fixup divider save_restore for floating point too; improve tests (#405) * fix pico_promote_common_scope_vars (#397) * add comment about using clk_gpout0 enable bit (Fixes #413) * pioasm: prevent double inclusion for C SDK generated headers (#417) * Add missing cast to uint32_t in hw_divider_u32_quotient for host (#436) * Optional feature to get the max level that has ever been held by a queue (#444) * Fix wrong format string in alarm_pool_dump_key (#437) * Add support for Arduino Nano RP2040 Connect (#425) * Add support for Arduino Nano RP2040 Connect * Add support for at25sf128a flash * Fix function-name misspelling (#443) * Update host multicore.h to match multicore.h in rp2_common (#439) * Implement `uart_write_blocking` and `uart_read_blocking` for host (#438) * Define `__STRING` for other compilers than MSVC in the host platform.h file (#434) * Prevent warnings about some unused parameters in pico_stdio_usb when building with -Wextra (#431) * Fix warnings about some unused parameters in pico_stdio_usb * Use `__unused` for the unused parameter in tud_descriptor_configuration_cb * Remove redundant inclusions of `pico/platform.h` * Define `void operator delete[](void *p, std::size_t n)` in new_delete.cpp (#430) * queue: make data pointers const in queue_try_add and queue_add_blocking (#423) * misc interp_ fixes (#428) * some typo fixes (#408) * Prevent the literal string DEBUG from being appended to some messages in CMake < 3.15 (#433) * Add function to get the currently selected channel (#451) * Add missing board detection macros (#448) * add board detection macros for Sparkfun & RPi Pico / VGA Board * dma_channel_transfer_[from/to]_buffer_now: added const volatile to read_addr and volatile to write_addr (#449) * Change the quick-start instructions to include installation of the (#92) * added spi_get_baudrate() + some consistency changes (#395) * Allow lengthening xosc startup delay with a compile option (#457) * Add hardware_gpio accessors for Schmitt, slew rate, drive strength (fixes #290) (#464) * Add some spin lock related doxygen * Move to Tinyusb 0.10.0 (#462) * Add usb device dpram to svd file. Fixes #351 (#465) * Add PICO_PANIC_FUNCTION define to allow replacement of the default panic function (#463) * Add missing DREQ_ definitions * store actual clock frequency in clock_configure (fixes #368) * Fix hw_is_claimed, and add xxx_is_claimed APIs * Add some PIO irq helper methods * Add DMA channel IRQ status getter and clear methods * Implement the correct PIO IRQ status/clear methods (good to have methods here as the h/w interrupt registers are super confusing) * fix pico_multicore dependencies * add missing wrapper func __aeabi_f2d * Further DMA/PIO IRQ API cleanup (and review fixes) * add PICO_INT64_OPS_IN_RAM flag * fix qtpy rp2040 uart rx rev B (#466) * Move to tinyusb 0.10.1 (upstream tinyusb repo) ($467) * Add gpio_set_irqover to match inover/outover/oeover (fixes #265) (#470) Co-authored-by: Andrew Scheller <andrew.scheller@raspberrypi.com> Co-authored-by: Christian Flach <cmfcmf.flach@gmail.com> Co-authored-by: Luke Wren <wren6991@gmail.com> Co-authored-by: Earle F. Philhower, III <earlephilhower@yahoo.com> Co-authored-by: majbthrd <majbthrd@gmail.com> Co-authored-by: Peter Lawrence <12226419+majbthrd@users.noreply.github.com> Co-authored-by: Brian Cooke <bdscooke@gmail.com> Co-authored-by: Scott Shawcroft <scott@tannewt.org> Co-authored-by: Michael Stoops <spam@michaelstoops.com> Co-authored-by: ZodiusInfuser <christopher.parrott2@gmail.com> Co-authored-by: Kirk Benell <36707344+kirk-sfe@users.noreply.github.com> Co-authored-by: Ha Thach <thach@tinyusb.org> Co-authored-by: Exr0n <spotyie@gmail.com> Co-authored-by: Joni Kähärä <joni.kahara@async.fi> Co-authored-by: Rafael G. Martins <rafael@rafaelmartins.eng.br> Co-authored-by: Jonathan Reichelt Gjertsen <jonath.re@gmail.com> Co-authored-by: Martino Facchin <m.facchin@arduino.cc> Co-authored-by: Rene <reneg973@gmail.com> Co-authored-by: Brendan <2bndy5@gmail.com> Co-authored-by: geurtv <48989893+geurtv@users.noreply.github.com> Co-authored-by: ewpa <34030942+ewpa@users.noreply.github.com> Co-authored-by: Dan Halbert <halbert@halwitz.org> Co-authored-by: Liam Fraser <liam@raspberrypi.com>

added spi_get_baudrate() + some consistency changes

ecc002c

- new function to query the current baudrate + consistency changes - return a boolean for functions with a "is; has; can" in name - return unsigned value for number of bytes read/written - some const correctness

Reneg973 mentioned this pull request May 11, 2021

For consistency reasons, add a function spi_get_baudrate() #390

Closed

lurch reviewed May 11, 2021

View reviewed changes

src/rp2_common/hardware_spi/include/hardware/spi.h Outdated Show resolved Hide resolved

lurch reviewed May 11, 2021

View reviewed changes

src/rp2_common/hardware_spi/include/hardware/spi.h Show resolved Hide resolved

lurch reviewed May 11, 2021

View reviewed changes

src/rp2_common/hardware_spi/spi.c Show resolved Hide resolved

Reneg973 added 2 commits May 11, 2021 13:03

fixed docu

f385948

Merge branch 'develop' into SPI-Helpers

ae375ab

kilograham requested changes May 11, 2021

View reviewed changes

lurch reviewed May 11, 2021

View reviewed changes

src/rp2_common/hardware_spi/include/hardware/spi.h Outdated Show resolved Hide resolved

Reneg973 added 2 commits May 12, 2021 23:53

Update spi.h

b43ac69

removed misleading docu

Merge branch 'SPI-Helpers' of https://github.com/Reneg973/pico-sdk in…

272bd55

…to SPI-Helpers

lurch reviewed May 12, 2021

View reviewed changes

src/rp2_common/hardware_spi/include/hardware/spi.h Outdated Show resolved Hide resolved

another misleading docu removed

05224d2

kilograham requested changes May 13, 2021

View reviewed changes

improved const correctness

5a043a2

Reneg973 force-pushed the SPI-Helpers branch from 43da962 to 5a043a2 Compare May 13, 2021 17:43

kilograham requested changes May 13, 2021

View reviewed changes

fixed kitchen_sink issues

b0e2b1e

reverted size_t return value to int

5b2f444

kilograham approved these changes Jun 1, 2021

View reviewed changes

kilograham added this to the 1.2.0 milestone Jun 1, 2021

kilograham merged commit 42cbdcb into raspberrypi:develop Jun 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added spi_get_baudrate() + some consistency changes #395

added spi_get_baudrate() + some consistency changes #395

Reneg973 commented May 11, 2021

lurch commented May 11, 2021

kilograham May 11, 2021

Reneg973 May 11, 2021

kilograham May 11, 2021

Reneg973 May 12, 2021

kilograham May 13, 2021

Reneg973 May 13, 2021 •

edited

Loading

Reneg973 May 13, 2021 •

edited

Loading

kilograham left a comment

kilograham May 13, 2021

kilograham left a comment

kilograham commented May 13, 2021

Reneg973 commented May 14, 2021

Reneg973 commented May 14, 2021

lurch commented May 14, 2021

kilograham commented May 14, 2021

kilograham commented May 14, 2021

Reneg973 commented May 14, 2021

Reneg973 commented May 14, 2021

kilograham commented May 14, 2021

lurch commented May 14, 2021

Reneg973 commented May 14, 2021

kilograham commented May 14, 2021

Reneg973 commented May 14, 2021

lurch commented May 14, 2021

Reneg973 commented May 14, 2021 •

edited

Loading

added spi_get_baudrate() + some consistency changes #395

added spi_get_baudrate() + some consistency changes #395

Conversation

Reneg973 commented May 11, 2021

lurch commented May 11, 2021

kilograham May 11, 2021

Choose a reason for hiding this comment

Reneg973 May 11, 2021

Choose a reason for hiding this comment

kilograham May 11, 2021

Choose a reason for hiding this comment

Reneg973 May 12, 2021

Choose a reason for hiding this comment

kilograham May 13, 2021

Choose a reason for hiding this comment

Reneg973 May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Reneg973 May 13, 2021 • edited Loading

Choose a reason for hiding this comment

kilograham left a comment

Choose a reason for hiding this comment

kilograham May 13, 2021

Choose a reason for hiding this comment

kilograham left a comment

Choose a reason for hiding this comment

kilograham commented May 13, 2021

Reneg973 commented May 14, 2021

Reneg973 commented May 14, 2021

lurch commented May 14, 2021

kilograham commented May 14, 2021

kilograham commented May 14, 2021

Reneg973 commented May 14, 2021

Reneg973 commented May 14, 2021

kilograham commented May 14, 2021

lurch commented May 14, 2021

Reneg973 commented May 14, 2021

kilograham commented May 14, 2021

Reneg973 commented May 14, 2021

lurch commented May 14, 2021

Reneg973 commented May 14, 2021 • edited Loading

Reneg973 May 13, 2021 •

edited

Loading

Reneg973 May 13, 2021 •

edited

Loading

Reneg973 commented May 14, 2021 •

edited

Loading