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

Adding examples/ethernet/basic, support mros2-esp32 #27

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yamati-kz
Copy link

We would like to make a proposal for Ethernet support.
Could you please delete the following file: platform/ethernet/CMakeLists.txt? I realize I was wrong.
I am merging three Kconfig.projbuild files. margefile.zip

#!/bin/bash
cat ./mros2esp32-network-select.txt \
    ~/esp/esp-idf/examples/ethernet/basic/main/Kconfig.projbuild \
    ~/esp/esp-idf/examples/ethernet/basic/components/ethernet_init/Kconfig.projbuild \
    > ./Kconfig.projbuild
if [ $? -eq 0 ]; then
    echo "Kconfig.projbuild has been generated. Merging the files."
else
    echo "Error: Failed to merge the files."
fi

We will copy the Kconfig.projbuild file. example ~/mros2-esp32/workspace/echoback_string/main/

workspace
├── echoback_string
│   ├── CMakeLists.txt
│   └── main
│       ├── CMakeLists.txt
│       ├── echoback_string.cpp
│       └── Kconfig.projbuild   # add copy
├── echoreply_string
├── m5stack_sample
├── pub_float32
├── pub_image
├── pub_long_string_sub_crc
├── pub_twist
├── sub_pose
└── sub_uint16

Handling of the Kconfig.projbuild during Ethernet support in the mros2-esp32 project #26

```
menu "mros2-esp32 network interface"
    choice
        prompt "mros2-esp32 network interface"
	    default MROS2_ESP32_NETIF_WIFI
            config MROS2_ESP32_NETIF_WIFI
                bool "WiFi"
            config MROS2_ESP32_NETIF_ETH_SPI
                bool "Ethernet-SPI"
	endchoice
endmenu
```
[examples/ethernet/basic/components/ethernet_init/Kconfig.projbuild](https://github.com/espressif/esp-idf/blob/master/examples/ethernet/basic/components/ethernet_init/Kconfig.projbuild)
[ethernet/basic/main/Kconfig.projbuild](https://github.com/espressif/esp-idf/blob/master/examples/ethernet/basic/main/Kconfig.projbuild)
@takasehideki
Copy link
Member

takasehideki commented Dec 29, 2024

@yamati-kz first of all, thank you so much for your awesome work!!
I feel very positive about merging your work into this repository. However, I wanna discuss the following points to enhance the future quality of this repository.

  • prepare (create) workspace/echoback_string_ethernet
    I think it is not so a general use case that we want to use Ethernet on the ESP32 family. So it may be good to prepare (create) the new directory for this case. Simply speaking, copy the typical exampleechoback_string, add an easy-to-understand name (i.e. _ethernet), and place the necessary file for this purpose (i.e. Kconfig.projbuild) here. My idea may make it easier to understand for newcomers who want to try this repository by Ethernet connection.
    Moreover, it is desirable to prepare README that represents the instruction to use an ethernet connection here. Also, I guess that you have referred (or just copied) files from somewhere for your implementation, so it is a good manner to include that information as the pointer as well.
    If you can prepare this, I would like to add a note to the README at the top of this repository that says, “If you want to use Ethernet communication, please refer to [this page]." If we want to use Ethernet communication in other samples, this directory and instruction will enable us to do it ourselves.

- consider to remove platform/ethernet/main/
I guess this repository is just an example of using an Ethernet connection, and these files (CMakeLists.txt and ethernet_example_main.[ch]) were just copied from the official repository for the ESP32 family (I'm not sure where this was). IOW, I think these files are not needed to compile and operate mros2-esp32.
Could you consider removing them from this PR? Let me know if we need these files for this repository.

-> understood!! I noticed that these files are needed! So I wanna withdraw this discussion,,,;(

Comment on lines 25 to +26
#include "wifi.h"

#include "ethernet_example_main.h"
Copy link
Member

@takasehideki takasehideki Dec 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code block can be modified as the below.

#ifdef CONFIG_MROS2_ESP32_NETIF_WIFI
#include "wifi.h"
#elif CONFIG_MROS2_ESP32_NETIF_ETH_SPI
#include "ethernet_example_main.h"
#else

I'm not sure this modification will work well, so I'm happy if you could check the operation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@takasehideki
I will conduct the verification and provide the results. Please allow me some time.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is working without any issues, as per @takasehideki -san's suggestion.
The flow remains the same, but the names have been changed. I have written the test cases in a way that makes the case distinctions clear.

#ifdef CONFIG_MROS2_ESP32_NETIF_WIFI
#include "wifi.h"
#elif CONFIG_MROS2_ESP32_NETIF_ETHERNET
#include "example_ethernet.h"
#else /* Not use Kconfig.projbuild */
#include "wifi.h"
#endif

@yamati-kz
Copy link
Author

@takasehideki
I understand the concept of the mros2-esp32 project. And I will consider it in detail.
I would be happy to discuss this in order to come up with a better direction.

@yamati-kz
Copy link
Author

@takasehideki san

I have placed the first version of the README in yamati-kz/mros2-esp32-eth1#1 (comment) . Would it be possible to discuss it in Japanese?

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.

2 participants