httpsrv fix + capacitive touch + remote syslog ++

[the0ne]

added fix for the httpsrv crashing while a "console" script is running moved the httpsrv config to the "Network services" menu added "make menuconfig" options to configure ssh server settings updated wifi callback handling according to 660d760 added a lua module to use the capacitive touch functionality of the esp32

for the hw design of capacitive touch buttons see https://github.com/espressif/esp-iot-solution/blob/master/documents/touch_pad_solution/touch_sensor_design_en.md

[the0ne]

added support for remote syslog logging. default rsyslog-IP 10.0.0.10 could later be set though sdkconfig ;-)

[the0ne]

@jolivepetrus please note https://github.com/the0ne/Lua-RTOS-ESP32/commit/a59247698017df05759d1096532f32c05e3129a6

[jolivepetrus]

@the0ne,

Many thanks for this PR!, unfortunately it is a very big one PR that affects many Lua RTOS parts, and it's a potential risk to merge the PR into the master branch.

Any possibility to split the PR into individual PR to make the work easy? The following will be great:

• Syslog --> 1 PR / 1 commit message
• Capacitive touch -> 1 PR / 1 commit message
• Http server -> 1 PR / 1 commit message
• Lua api -> 1 PR / 1 commit message

[the0ne]

That would take quite a while i think... I'll first try to resolve the conflicts with your latest commit. Would it maybe be possible to review the individual commits? I have tried to explain all change reasons in each individual commit.

[jolivepetrus]

@the0ne, major part of the PR has been accepted and merged to the master branch.

In the following days I will review the pending comments.

Many thanks for your work. Great!.

[jolivepetrus]

Comments about commit 97b5a1180c692e62f3a8a2574c86f366cb0db69d:

We have to work more on this. My experience is that it is unlikely to find errors in the Lua code, but due to the Read Only tables features, and that Lua RTOS can run Lua code into multiple threads, maybe some bug is introduced.

In any case, all changes introduced in Lua by the Whitecat Team are well identified to ensure that we will replicate the changes in future Lua upgrades. If we have to make a change, the way to act is as follows:

#if !LUA_USE_ROTABLE
new code introduced by the Whitecat Team
#else
original Lua code
#endif

You have an example in lapi.c

[jolivepetrus]

Comments about commit cf3ae3f5d976caadac870763b61dfc3cb1bd4fec:

Please, move the following code:

        syslog(LOG_ERR, "%s\n", "not enough memory");
        if (! (getlogstat() & LOG_CONS)) {
            lua_writestringerror("%s\n", "not enough memory");
        }

... into lua_writestringerror.

The same happens in other commits. I imagine that you are finding a way to know the errors generated by Lua RTOS remotely, and that's great, but we need to ensure that if something fails we will have the message in the log without having to add extra code to our applications.

[jolivepetrus]

Comments about commit f6dfac7c5429de264caffa1ed2ad241f04f88d95:

• Changes related to Lua are merely cosmetic changes. It's not worth investing time in that, it would complicate the work of upgrading Lua to new versions.

• Changes related to Lua RTOS. Be careful, sometimes an argument is defined with int8_t because -1 us used for express "not used", "not set", etc ...

[the0ne]

Comments about commit 97b5a11:

We have to work more on this. My experience is that it is unlikely to find errors in the Lua code, but due to the Read Only tables features, and that Lua RTOS can run Lua code into multiple threads, maybe some bug is introduced.

In any case, all changes introduced in Lua by the Whitecat Team are well identified to ensure that we will replicate the changes in future Lua upgrades. If we have to make a change, the way to act is as follows:

#if !LUA_USE_ROTABLE
new code introduced by the Whitecat Team
#else
original Lua code
#endif

You have an example in lapi.c

Yes this change is definitely needed on the ESP32 platform. Committed as abe43c6

[the0ne]

Comments about commit f6dfac7:

* Changes related to Lua are merely cosmetic changes. It's not worth investing time in that, it would complicate the work of upgrading Lua to new versions.

* Changes related to Lua RTOS. Be careful, sometimes an argument is defined with int8_t because -1 us used for express "not used", "not set", etc ...

Agreed 100% - this was merely to make my static code analysis tool stop complaining. I tried to verify each and every case and I'm positive that I didn't fix anything wrong. If you want to discard this commit I'm absolutely fine with that :-)

[the0ne]

Comments about commit cf3ae3f:

Please, move the following code:

      syslog(LOG_ERR, "%s\n", "not enough memory");
      if (! (getlogstat() & LOG_CONS)) {
          lua_writestringerror("%s\n", "not enough memory");
      }

... into lua_writestringerror.

The same happens in other commits. I imagine that you are finding a way to know the errors generated by Lua RTOS remotely, and that's great, but we need to ensure that if something fails we will have the message in the log without having to add extra code to our applications.

I totally get your point ... but ...

` if (pname) syslog(LOG_ERR, "%s: %s\n", pname, msg); else syslog(LOG_ERR, "%s\n", msg);

if (! (getlogstat() & LOG_CONS)) { if (pname) lua_writestringerror("%s: ", pname); lua_writestringerror("%s\n", msg); } `

as you see lua_writestringerror is not always used in a way where one message is printed as a whole but it's called multiple times to print only one message. That was the reason I had decided for an additional call outside lua_writestringerror. To avoid having the message printed twice (once from syslog, once from lua_writestringerror) I needed to add the if-condition. ^^ I have re-implemented this to have only one call to lua_writestringerror in all cases, so it now was possible to put the syslog call inside a new function lua_writestringerror.

main issue for me is that the latest update from trunk has lots of issues with file system creation. spiffs doesn't recognize the correct base address, lfs is being built (and breaks the build) despite it's disabled in config, spiffs takes the wrong base folder, etc.

[the0ne]

hacked some config files to be able to flash a valid file system. lua_writestringerror looks good.

[the0ne]

I have reverted the cosmetic lua changes that the static code analysis tool had complained about. please note that I also fixed line ending issues in uart.c that it seems have not yet been taken over.

using a compare tool between wcb/master and this pull request I see a number of differences that should go into master. please also note a81f97d which should not get lost.

for e83bffa you might prefer a different solution or a different return value. but please definitely keep the #ifdef guard in components/sys/vfs/ramfs.c

[jolivepetrus]

Comment about commit e83bffaf30e5b9a883fb1d5dd7b7520ccdc0abb3:

POSIX specifies that the telldir function set errno to EBADF (http://man7.org/linux/man-pages/man3/telldir.3.html), if telldir fails. Checked in OSX, when the DIR stream reaches the end, a value z < 0 is returned, and errno is set to EBADF.

Replaced with commit 207bf6332adbb78a35f28a948bf47a8410c3f75a, which solves a related issue when telldir is < 0.

[the0ne]

Now fixed for ramfs in the same way.

[jolivepetrus]

Comments about commit fc9f81e3ab6080991e86f5329efa07a9f58b9203:

The proposed implementation for RAM FS in the master branch is POSIX-compliance:

    offset = ramfs_telldir(&fs, dir->fs_dir);
    if (offset < 0) {
        errno = ramfs_to_errno(result);
        return -1;
    }

... in ramfs_telldir, RAMFS_ERR_BADF is returned when the directory stream reaches the end:

ramfs_off_t ramfs_telldir(ramfs_t *fs, ramfs_dir_t *dir) {
    if (dir->offset < 0) {
        return RAMFS_ERR_BADF;
    }

    return dir->offset;
}

, and then ramfs_to_errno translates RAMFS_ERR_BADF to EBADF.

Commit fc9f81e3ab6080991e86f5329efa07a9f58b9203 is not required.

[the0ne]

Comments about commit fc9f81e:

The proposed implementation for RAM FS in the master branch is POSIX-compliance:

    offset = ramfs_telldir(&fs, dir->fs_dir);
    if (offset < 0) {
        errno = ramfs_to_errno(result);
        return -1;
    }

... in ramfs_telldir, RAMFS_ERR_BADF is returned when the directory stream reaches the end:

ramfs_off_t ramfs_telldir(ramfs_t *fs, ramfs_dir_t *dir) {
    if (dir->offset < 0) {
        return RAMFS_ERR_BADF;
    }

    return dir->offset;
}

, and then ramfs_to_errno translates RAMFS_ERR_BADF to EBADF.

Commit fc9f81e is not required.

Sorry, maybe I just don't get it. This is the code:

static long vfs_ramfs_telldir(DIR *dirp) {
    vfs_dir_t *dir = (vfs_dir_t *)dirp;
    int result;

    ramfs_off_t offset;

    offset = ramfs_telldir(&fs, dir->fs_dir);
    if (offset < 0) {
        errno = ramfs_to_errno(result);
        return -1;
    }

    return (long)offset;
}

So definitely result is used as a parameter despite it's never assigned a value before. How can that be correct?

[jolivepetrus]

Ahh, ok,

solved in https://github.com/whitecatboard/Lua-RTOS-ESP32/commit/37af18633fa43f629ac40209ab9c9879a3362438

[the0ne]

I notice that the following don't work anymore with the latest upstream versions:

• wildcard ls - e.g. dir set* (re-added as d791a4e)
• history (now probably should be done via ramfs - but looks like one can either have a file system or ramfs)
• automatic http preprocessor re-preprocession (on fs without timestamp)

Have these been removed intentionally? @jolivepetrus Can you please confirm?

[the0ne]

added some fixes for mqtt and ssh-server @jolivepetrus make sure to pick d6699cd before doing any secured mqtt

[the0ne]

I've removed the static code-analysis based improvements from pwm source and merged in changes from trunk.

Please note that the added check in mqtt is still required to make sure the same callback can only be added for a specific topic once. Else the same lua function would be called for one message more than once which definitely isn't what the user would expect.

As mentioned before changes in uart.c are only the replacment of tabs to spaces. OTA has greatly improved use cases and it's output now behaves/looks like the esp-idf flash tooling. syslog now features remote-logging which is one of the main changes left to pull.

[jolivepetrus]

Hi @the0ne,

I don't remember "Please note that the added check in mqtt is still required to make sure the same callback can only be added for a specific topic once. Else the same lua function would be called for one message more than once which definitely isn't what the user would expect.". It is not included in the master tree?.

[the0ne]

Hi @jolivepetrus,

this is a rather new addition, see the first ~25 changed lines in https://github.com/whitecatboard/Lua-RTOS-ESP32/pull/184/files#diff-33660e1ae90f207275f7c7e54654858f the remaining lines are fixes to the formatting of that file.

The addition greatly simplifys lua programming in a way that it's no longer necessary to check if the lua code has already subscribed a specific topic to a specific lua callback function. It's now safe to do the following:

function mqtt_receive(len, message)
    print("new message received")
    print("message length: "..len)
    print("message: "..message)
end

client:subscribe("foo/bar/topic", mqtt.QOS0, mqtt_receive)
client:subscribe("foo/bar/topic", mqtt.QOS0, mqtt_receive)
client:subscribe("foo/bar/topic", mqtt.QOS0, mqtt_receive)

Without the added code, the function would have been called a number of times for only one received mqtt message on the same topic. Now, it's safe to just call client:subscribe again without having to worry if it had been called before.

It's not yet included in the master tree as it's rather new. With "It's not yet included in the master tree" I mean you haven't cherry-picked it yet.

[the0ne]

@jolivepetrus updated to latest

[the0ne]

@jolivepetrus this looks like a major step in terms of reliability of captivedns + http-server

[jolivepetrus]

@the0ne,

Commit https://github.com/whitecatboard/Lua-RTOS-ESP32/commit/9c757a84beaa5e5cdd6a3605cb09de5d90bb59c1 removes a regression introduced by https://github.com/whitecatboard/Lua-RTOS-ESP32/commit/6ebcdfed73279a7c03a3905de46cae644c943cad. I have seen some commit today on your PR that tries to remove the bug, but I think that commit https://github.com/whitecatboard/Lua-RTOS-ESP32/commit/9c757a84beaa5e5cdd6a3605cb09de5d90bb59c1 solves the regression doing fewer changes, and ensures backward compatibility with software developed by others that use Lua RTOS low APIs.

[the0ne]

@the0ne,

Commit 9c757a8 removes a regression introduced by 6ebcdfe. I have seen some commit today on your PR that tries to remove the bug, but I think that commit 9c757a8 solves the regression doing fewer changes, and ensures backward compatibility with software developed by others that use Lua RTOS low APIs.

I understand that Espressif has noticed it doesn't make sense to have the CPU counted in Hz. So they updated their interface. I believe we should do the same and count CPU speed in MHz. That's why when replacing the deprecated interface in https://github.com/whitecatboard/Lua-RTOS-ESP32/commit/6ebcdfed73279a7c03a3905de46cae644c943cad I intentionally didn't multiply MHz back to Hz. In https://github.com/whitecatboard/Lua-RTOS-ESP32/pull/184/commits/b79c8e0430dc3236f5dca85a7c7feeb308531d90 I fixed all "direct" internal usages of cpu_speed() so that the calculation results are as before the deprecation.

If you prefer then I can add a new interface with MHz, keep the old interface on Hz and "deprecate" it - but I really believe we should go with MHz.

[jolivepetrus]

@the0ne,

Ok, I propose the following:

• cpu_speed, mark it as deprecated, should return speed in Hz.
• add cpu_speed_hz function, should return speed in Hz.
• add cpu_speed_mhz function, should return speed in MHz.
• in cpu.speed function use cpu_speed_mhz. This function now, that is the used by high-level programmers returns MHz and not Hz (wiki was updated few days before).
• the use in other parts of the cpu_speed function can be either use cpu_speed_hz or cpu_speed_mhz, we only need to ensure that not overflows occur, so are used by critical parts, such as rtm.

[the0ne]

@the0ne,

Ok, I propose the following:

* cpu_speed, mark it as deprecated, should return speed in Hz.

* add cpu_speed_hz function, should return speed in Hz.

* add cpu_speed_mhz function, should return speed in MHz.

* in cpu.speed function use cpu_speed_mhz. This function now, that is the used by high-level programmers returns MHz and not Hz (wiki was updated few days before).

* the use in other parts of the cpu_speed function can be either use cpu_speed_hz or cpu_speed_mhz, we only need to ensure that not overflows occur, so are used by critical parts, such as rtm.

Sounds good!

Btw: Please check https://github.com/the0ne/Lua-RTOS-ESP32.wiki.git for the updated documentation of:

• CPU-module.md
• File-system.md
• File-systems.md
• How-to-transfer-files.md
• LoRa-WAN-node-module.md
• Lua-RTOS-&-board-status.md
• Lua-RTOS-configuration.md
• MQTT-module.md
• Net-module.md
• Network-services.md
• Sleep.md

[the0ne]

@the0ne,

Ok, I propose the following:

* cpu_speed, mark it as deprecated, should return speed in Hz.

* add cpu_speed_hz function, should return speed in Hz.

* add cpu_speed_mhz function, should return speed in MHz.

* in cpu.speed function use cpu_speed_mhz. This function now, that is the used by high-level programmers returns MHz and not Hz (wiki was updated few days before).

* the use in other parts of the cpu_speed function can be either use cpu_speed_hz or cpu_speed_mhz, we only need to ensure that not overflows occur, so are used by critical parts, such as rtm.

committed as https://github.com/the0ne/Lua-RTOS-ESP32/commit/137efa3f70ee0b1980ead9a16c5cc420eeec52e0

[jolivepetrus]

@the0ne,

I can't find the reference commit (committed as 137efa3.

[the0ne]

Link updated to https://github.com/the0ne/Lua-RTOS-ESP32/commit/137efa3f70ee0b1980ead9a16c5cc420eeec52e0. The commit is linked just above my last comment as well, that's where i took the link from... On my system both links show the diff.

[jolivepetrus]

@the0ne,

Commit https://github.com/whitecatboard/Lua-RTOS-ESP32/pull/184/commits/98ea87731c7e6c302b8789cf4ebe4fa41118bd93 can't be accepted, because it's incompatible with the Whitecat IDE, and incoherent with other Lua RTOS parts.

Reason is the use of syslog to show new errors. If a new error is required, it must be created in the corresponding driver, in this case sys/drivers/wifi.c, and return the error in the Lua Module using "return luaL_driver_error(L, wifi_check_error(rc))".

[the0ne]

@jolivepetrus thanks for the hint!

I'm trying to fix it but I'm not sure I totally understand what you mean.

Do you refer to these (three) blocks?

        int rc = get_file_content(cacert_file, &enterprise_cert_info.cacert, &enterprise_cert_info.cacert_len);
        if (ESP_OK != rc) {
            syslog(LOG_ERR, "wifi_setup_enterprise: get_file_content(cacert) error %s\n", esp_err_to_name(rc));
            return luaL_driver_error(L, wifi_check_error(rc));
        }

There, the return luaL_driver_error(L, wifi_check_error(rc)) call will make sure that the correct error (ESP_ERR_INVALID_ARG, ESP_ERR_NO_MEM or ESP_FAIL) will be triggered. Is that what you requested for compatibility with the Whitecat IDE? Please note that the syslog message is only there to further help the user by showing which of the certificates could not be read. But actually, if you want, that syslog could of cause be removed. The error will be triggered without it as well.

It's just that 9 additional error messages didn't seem to make sense to me, especially in the case of ESP_ERR_NO_MEM. But if you think it's required to show the user during the loading of which of the certificates the error occurred, then it's fine for me and I will add the individual error messages to the driver of cause.

Please confirm - Thanks in advance!

[the0ne]

I have split this up into several (>10) seperate PRs. Let's try to get those done - afterwards let's close this monster :-) At least one more PR will be required: httpsrv

[jolivepetrus]

@the0ne,

Is this PR still required? Can be closed?

[the0ne]

@the0ne,

Is this PR still required? Can be closed?

I'll close it once each change is either in the mainline or decided to not include. So after the PRs and the doku is done, I'll do another diff, verify it's content and only after that close this. If you want me to close it now, then that's also ok. please let me know in this case.

[the0ne]

Currently no time to do a proper cleanup. Closing this PR now, final compare + cleanup will be done later.