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

[BUG] Espressif Pulse Counter changes and bugs in Quadrature Encoder logic #15319

Open
1 task done
ppisa opened this issue Dec 23, 2024 · 2 comments
Open
1 task done
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: Drivers Drivers issues OS: Linux Issues related to Linux (building system, etc) Type: Bug Something isn't working

Comments

@ppisa
Copy link
Contributor

ppisa commented Dec 23, 2024

Description / Steps to reproduce the issue

The changes in "Feature/esp pulse cnt" #15079 authored by @eren-terzioglu seems to be completely broken to me. @Vajnar has invested time to provide reasonable support to enable ESP32-C6 platform usable for robotic experiments, see slides from LinuxDays 2024 presentation Low-Cost Motor and Tiny Robots Control with RISC-V Based Espressif MCUs and NuttX. That is larger effort which I coordinate.

There are obvious errors in the code, i.e. limit return value of IOCTL that it cannot return negative value tested on next line in the function esp_position(). Missing sign extension from 16-bits to ensure correct computation of the 32-bit value.

There should be discussion about need to add spinlocks, because the code of position extension has been designed such way, that it did not need locks even for concurrent execution and still provides correct results. But when changed to HAL based one then it seems that that that property has been lost.

Another problem is, that quick attempt to build the NuttX for ESP32-C6 within my standard environment has failed for me now

tools/configure.sh esp32c6-devkitc:nsh
make

leads to

*** No rule to make target 'chip/esp-hal-3rdparty/components/esp_rom/patches/esp_rom_hp_regi2c_esp32c6.c', needed by '.depend'.  Stop.

It can be driven by some need to install some specific Espressif environment, but that is bad again.

It is bad that NuttX does not provide MAINTAINERS and the culture to prune ordinal code authors from the files header means that there is no way how to find who worked and invested effort into which drivers, code... That leads to the state where original authors are not informed when somebody changes the code, the same the copies of drivers cannot be identified. So the problem is wider than this single change. We can see how our initial through through CAN driver in ESP32-C3 driver has been replace or evolved from generic driver working with all clocks and bitrates combinations into solution with large set of defines required for each combination etc... The drivers are hard to review because they are divided in HAL and NuttX part etc...

So for #15079 i do not see if/what is change in pcnt_ll_get_count() function etc....

On which OS does this issue occur?

[OS: Linux]

What is the version of your OS?

Debian 12.8

NuttX Version

master

Issue Architecture

[Arch: risc-v], [Arch: xtensa]

Issue Area

[Area: Drivers]

Verification

  • I have verified before submitting the report.
@ppisa ppisa added the Type: Bug Something isn't working label Dec 23, 2024
@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: Drivers Drivers issues OS: Linux Issues related to Linux (building system, etc) labels Dec 23, 2024
@gk351
Copy link

gk351 commented Dec 23, 2024

``

@acassis
Copy link
Contributor

acassis commented Dec 25, 2024

@eren-terzioglu @tmedicci @fdcavalcanti please take a look ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: Drivers Drivers issues OS: Linux Issues related to Linux (building system, etc) Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants