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

esp32 support #182

Merged
merged 32 commits into from
Jun 27, 2024
Merged

esp32 support #182

merged 32 commits into from
Jun 27, 2024

Conversation

Xavrax
Copy link
Contributor

@Xavrax Xavrax commented Apr 16, 2024

feat: ESP32 platform support
Provided support for ESP32 devices via ESP-IDF framework

feat: MBedTLS support
Provided support for MBedTLS library used within esp32 platform

refactor: strcpy instead of strncpy when it is safe
Replace strncpy with strcpy in blocks where it is safer to be used.

Copy link

@blake-spangenberg blake-spangenberg left a comment

Choose a reason for hiding this comment

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

Greetings,

I'm working with Insteon and am eagerly awaiting the completion of esp support for the PubNub SDK. Seeing as this port is nearing completion, I have experimented with integrating it into our project have have some feedback as a result of that experiment:

  • In core/pubnub_alloc_static.c, pubnub_alloc relies on local static FreeRTOS mutexes, but there is nothing in that file which calls pubnub_mutex_init on them, thus causing an exception at runtime for our project:
pubnub_t *pubnub_alloc(void)
{
    size_t i;

    for (i = 0; i < PUBNUB_CTX_MAX; ++i) {
        // Something like this is needed...
        // if (!m_aCtx[i].monitor) {
        //    pubnub_mutex_init(m_aCtx[i].monitor)
        // }
        pubnub_mutex_lock(m_aCtx[i].monitor);
        if (m_aCtx[i].state == PBS_NULL) {
            m_aCtx[i].state = PBS_IDLE;
            pubnub_mutex_unlock(m_aCtx[i].monitor);
            return m_aCtx + i;
        }
        pubnub_mutex_unlock(m_aCtx[i].monitor);
    }

    return NULL;
}
  • Would you consider incorporating this into espressif’s Component Registry? Their guide is provided here. This would be a great opportunity for you as it could make your service much more discoverable to those exploring the ESP32 for future projects. For us, it would greatly simplify the process of keeping up-to-date with the latest release of your SDK.

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@Xavrax
Copy link
Contributor Author

Xavrax commented Jun 24, 2024

Hello @blake-spangenberg !
At the end of the week I've changed a little bit a source to test the SDK over the qemu emulator.
I was able to test our pub/sub utility without any errors.
Right now I'm polishing the code to ensure that it works with other features and remove all the magic hacks and totos I've made during the process.

We're planning to release it this week but if you will notice any other issues with the code - let us know about it. We want to be sure that our product satisfies our customer needs <3.

@Xavrax
Copy link
Contributor Author

Xavrax commented Jun 24, 2024

Would you consider incorporating this into espressif’s Component Registry? Their guide is provided here.

That's a great idea. I will take a look into that and I will try to satisfy all the needs required by the registry.

@Xavrax
Copy link
Contributor Author

Xavrax commented Jun 24, 2024

@blake-spangenberg
We've asked our legals if we're okay with this 3rd party service.
I will let you know about the decision as soon as it will be made.

Until then you will need to rawly git clone the repository.
(hopefully they will allow us to publish our repo there and the SDK will be available at the registry to make the developer experience at the highest level - we've made same thing for rust's SDK so it shouldn't be a problem).

@Xavrax Xavrax marked this pull request as ready for review June 26, 2024 12:00
Copy link
Contributor

@parfeon parfeon left a comment

Choose a reason for hiding this comment

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

LGTM!

There are few small questions.

core/pubnub_generate_uuid.c Outdated Show resolved Hide resolved
freertos/pubnub_config.h Outdated Show resolved Hide resolved
lib/sockets/pbpal_handle_socket_error.c Outdated Show resolved Hide resolved
mbedtls/pbpal_connect_mbedtls.c Outdated Show resolved Hide resolved
@Xavrax Xavrax requested a review from blake-spangenberg June 27, 2024 07:47
@Xavrax
Copy link
Contributor Author

Xavrax commented Jun 27, 2024

@pubnub-release-bot release

@Xavrax Xavrax merged commit bb0ff5d into master Jun 27, 2024
6 checks passed
@Xavrax Xavrax deleted the check/esp32-component branch June 27, 2024 17:37
@pubnub-release-bot
Copy link
Contributor

🚀 Release successfully completed 🚀

@blake-spangenberg
Copy link

Can confirm publish/subscribe is working on our esp32-based hardware platform. Thanks for the hard work everyone!

We'll be in touch if there's any problems or follow-up questions.

@Xavrax
Copy link
Contributor Author

Xavrax commented Jun 28, 2024

Thank you for the confirmation and have fun with our SDK <3

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.

4 participants