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

move to new esp rmt device driver: resolves #28 #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mjcross
Copy link

@mjcross mjcross commented Apr 6, 2023

Proposed changes to integrate Espressif's new RMT driver

@mjcross
Copy link
Author

mjcross commented Apr 6, 2023

As promised, a new from-scratch implementation of owb_rmt.c et al that talks to Espressif's new RMT driver.

  • works with the existing unmodified esp32-ds18b20-example code
  • passes all the testing I've done so far (8 sensors on a ~2 metre bus)
  • not yet tested in the parasitic power configuration (but haven't touched any of that code)

The public API should be backward compatible but the tx_channel and rx_channel parameters don't do anything any more as the new driver removed these from user space.

There are also a couple of new public API functions: driver->read_bytes() and driver->write_bytes() added to owb.h because with the RMT it's slightly more efficient to handle multiple bytes at a time. They can handle up to 8 bytes at a time. I've left it up to you whether you want to hook them into your owb_read_bytes() and owb_write_bytes() (they are not at the moment); but the driver's 'bits' functions do automatically use them if called with nbits = 8.

@mjcross
Copy link
Author

mjcross commented Apr 6, 2023

Just shout if you want any tweaks.

@DavidAntliff
Copy link
Owner

@mjcross awesome, thanks for this. I will take a closer look soon.

Please don't be disheartened if you don't hear from me for a little while, I will get to this.

Quick question - if someone uses this with the older ESP-IDF, prior to the RMT deprecation, does it fail (to compile, or run) in an informative way?

@mjcross
Copy link
Author

mjcross commented Apr 11, 2023

@DavidAntliff no worries - I mainly did the updates because I use your lib in a couple of home projects but if others can benefit then that's a real bonus. FWIW I've already pushed something similar to my esp32_onewire repo that reproduces my API for the RPI Pico.

Good point about building with an an outdated IDF. At present it will just fail to compile but it would be far better to check IDF_VERSION_MAJOR in CMake. I'll take a look at that.

Could this also be a good time to strip out the old GPIO stuff? It would certainly simplify the project internally.

@CJCombrink
Copy link

CJCombrink commented Jul 17, 2023

@mjcross Nice work with the pr.

If I try to use it (drop in replacement) I get the following compiler error

C:/.../managed_components/esp32-owb/include/owb_rmt.h:77:32: error: unknown type name 'rmt_channel_t'
   77 |                                rmt_channel_t tx_channel, rmt_channel_t rx_channel);
      |                                ^~~~~~~~~~~~~
C:/.../managed_components/esp32-owb/include/owb_rmt.h:77:58: error: unknown type name 'rmt_channel_t'
   77 |                                rmt_channel_t tx_channel, rmt_channel_t rx_channel);

If I replace it with int (as in the source file) I get a new compiler error:

C:/.../managed_components/esp32-owb/include/owb_rmt.h:97:3: error: conflicting declaration 'typedef enum rmt_channel_t rmt_channel_t'        
   97 | } rmt_channel_t;
      |   ^~~~~~~~~~~~~
In file included from C:/Users/<>/esp/esp-idf/components/driver/include/driver/rmt_common.h:11,
                 from C:/Users/<>/esp/esp-idf/components/driver/include/driver/rmt_tx.h:12,
                 from C:/.../managed_components/esp32-owb/include/owb_rmt.h:41,
                 from C:/.../managed_components/esp32-owb/include/owb.h:324,
                 from C:/.../managed_components/esp32-ds18b20/include/ds18b20.h:37,
                 from C:/.../main/drivers/DS18B20Driver.cpp:10:
C:/Users/<>/esp/esp-idf/components/driver/include/driver/rmt_types.h:22:16: note: previous declaration as 'struct rmt_channel_t'
   22 | typedef struct rmt_channel_t *rmt_channel_handle_t;
      |                ^~~~~~~~~~~~~

I am using ESP-IDF 5.0.3.

Am I doing something wrong?

Update/Edit: The following steps allowed me to compile and test the code

  1. Replace rmt_channel_t with int in the header (owb_rmt.h).
  2. Comment out the typedef enum {...} rmt_channel_t from the header (owb_rmt.h)
  3. Replace any reference to the enums in my code with 0

After the above changes my code works as before but with the new driver

CJCombrink added a commit to CJCombrink/esp32-owb that referenced this pull request Jul 18, 2023
@mjcross
Copy link
Author

mjcross commented Jul 19, 2023

I get the following compiler error
C:/.../managed_components/esp32-owb/include/owb_rmt.h:77:32: error: unknown type name 'rmt_channel_t'

@CJCombrink Sounds like you're picking up the original version of include/owb_rmt.h rather than the one from the PR (link).

@DavidAntliff do you think you'll be likely to merge the PR at some point?

The PR version of include/owb_rmt.h has rmt_channel_t defined as follows, in lines 67-97:

/**
 * @brief Legacy RMT channel IDs.
 * 
 * These are no longer used for anything. They are defined here purely so
 * that legacy code can still compile.
 */
typedef enum {
    RMT_CHANNEL_0,  /*!< RMT channel number 0 */
    RMT_CHANNEL_1,  /*!< RMT channel number 1 */
    RMT_CHANNEL_2,  /*!< RMT channel number 2 */
    RMT_CHANNEL_3,  /*!< RMT channel number 3 */
#if SOC_RMT_CHANNELS_PER_GROUP > 4
    RMT_CHANNEL_4,  /*!< RMT channel number 4 */
    RMT_CHANNEL_5,  /*!< RMT channel number 5 */
    RMT_CHANNEL_6,  /*!< RMT channel number 6 */
    RMT_CHANNEL_7,  /*!< RMT channel number 7 */
#endif
    RMT_CHANNEL_MAX /*!< Number of RMT channels */
} rmt_channel_t;

@CJCombrink
Copy link

CJCombrink commented Jul 19, 2023

@mjcross
I will confirm, but a quick check showed that in the current release/v5.0/components/driver/include/driver/rmt_types.h#L22
there is this line that conflicts

typedef struct rmt_channel_t *rmt_channel_handle_t;

If you follow the includes in my original message you will see how it gets there.

The includes are:

  1. owb_rmt.h#L41
    #include "driver/rmt_tx.h"
    
  2. driver/rmt_tx.h#L12
    #include "driver/rmt_common.h"
    
  3. driver/rmt_common.h#L11
    #include "driver/rmt_types.h"
    
  4. driver/rmt_types.h#L22
    typedef struct rmt_channel_t *rmt_channel_handle_t;
    

@mjcross
Copy link
Author

mjcross commented Jul 19, 2023

@CJCombrink
Just checking my build environment for that PR locally (but it's vanilla so it's probably the same as yours). Please could you check to see that your version of esp32-owb/include/owb_rmt.h has the typedef enum{...} rmt_channel_t in lines 67-85 ?

@mjcross
Copy link
Author

mjcross commented Jul 19, 2023

@CJCombrink
OK I've checked my local build environment for the PR and I've got the same incudes as you listed (here) so that's not the issue.

@CJCombrink
Copy link

@mjcross Can you reproduce?
One thing to note perhaps is that I am compiling with a C++ compiler. I migrated my project from C to C++. Maybe the C++ compiler is a bit more strict in this regard.

@mjcross
Copy link
Author

mjcross commented Jul 19, 2023

@CJCombrink wrote:

Can you reproduce?

Tried a full clean build, and no errors... as you say, perhaps it's the compiler.

I'm using:

gcc --version
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: x86_64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

@CJCombrink
Copy link

This is what I have

C:/Users/<>/.espressif/tools/xtensa-esp32-elf/esp-2022r1-11.2.0/xtensa-esp32-elf/bin/xtensa-esp32-elf-g++.exe --version                                            
xtensa-esp32-elf-g++.exe (crosstool-NG esp-2022r1) 11.2.0                                                                                                                    
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants