Skip to content

Conversation

marcelstoer
Copy link
Member

As said earlier I don't know whether this should land on master or release. @nwf your advice required here.

nwf and others added 30 commits June 10, 2020 02:14
It's not clear that this ever worked, AFAICT nobody uses it, and it's an
old version of the sqlite3 engine at this point.  Absent a maintainer,
let's just get rid of it.
It has long been superseded by the rfswitch module
This file seems to have been missed by 35a266e
Otherwise we might truncate precision with small floats and that's
unlikely to work out well for anyone.
connecting to server.

Inside af426d0, the `mqtt_socket_timer`
function was modified so that instead of checking the presense of
allocated `mud->pesp_conn` structure, `mud->connected` field was used
on determining if the timer need to be disarmed.

However, this is not entirely correct. If the TCP socket is actively
connecting and haven't timed out yet, then `mud->connected` is also
`false` and the timer will think the connection is broken and
disarms itself. This has two consequences:

* The connection timeout counter is no longer decremented and checked
* After connection succeeds, keepalive heartbeat is no longer being
  sent (#3166). This is particularly noticeable in MQTT over TLS
  connections, because those usually takes longer than 1 second
  to finish and the timer would had chance to execute before connection
  is established

This commit checks the presense of `pesp_conn->proto.tcp` pointer
instead, which was allocated in the same place as the (old) `pesp_conn`
struct, and according to my test indeed fixes the above issue.
* DHT module: fix the handling of negative temps.

The macro handling the conversion from the 2 bytes buffer to a double
was handling negative values by checking the sign bit and taking the
negative value of the number minus the sign bit.
Unfortunately this does not work as the negative values are represented
in 1's complement, so for instance -1 was becoming -32767
  * +1 = b0000_0000_0000_ 0001
  * -1 = 1111_1111_1111_1111

This replace the spacial code with a signed 16 bits value.

* Refactoring: removes some code duplication.

* Fixed the conversion of the 8/16 bits values

Co-authored-by: Marco Dondero <[email protected]>
I've not been able to get the mqtt `connfail` callback to work.

I'm consistently receiving `method not supported` errors:
```
application.lua:53: method not supported
stack traceback:
        [C]: in function 'on'
        application.lua:53: in main chunk
        [C]: in function 'dofile'
        init.lua:18: in function <init.lua:6>
```

Example code:
```
function on_connection_failed(client, reason)
    print("mqtt connection failed: " .. reason)
end

m:on("connfail", on_connection_failed)
```

I believed this to be caused by the incorrect length comparison for `connfail`
that is updated here.

Once I changed that, the error went away, however the callback was never called.

I believe the callback was never called because of an incorrect assignment.

However, I saw this somewhat confusing description in the docs so this
assignment may be expected?
> The second (failure) callback aliases with the "connfail" callback available through :on(). (The "offline" callback is only called after an already established connection becomes closed. If the connect() call fails to establish a connection, the callback passed to :connect() is called and nothing else.)
In case a custom SPIFFS location was defined above, don't overwrite it.
…3222)

* Change struct to use integers. This is slightly more complex as we have to deal with Unsigned 32-bit integers (that aren't lua integers)
* Use int64 in struct rather than double.
* Fix sjson to do the right things in LUA5.3 with integers and floats
* SoftUART fixes:

- Simplify code by using lua_L* functions and using userdata properly
- Fix some edge-cases
- Add more examples to documentation

* Don't de-register interrupt hook if there is more RX instances

* More bug fixes and registering simplification with luaL_reref and unref2

* Correct documentation of SoftUART module
Redo last change
@nwf
Copy link
Member

nwf commented Sep 9, 2020

I think we should land it on release and then bump master to match (easily done from the command line: git push origin origin/release:master) as we are currently keeping both labels for downstream consumers (notably RTD, right?).

@marcelstoer
Copy link
Member Author

Fine for me. Who's willing to approve and merge (but not squash 😉) this?

Oh, and I just noticed this will shrink our LoC significantly: +2,942 −218,841 😄 👍

@nwf nwf merged commit 64bbf00 into release Sep 11, 2020
@HHHartmann
Copy link
Member

HHHartmann commented Sep 11, 2020

I would like to add a win luac.cross to the announcement of the release. I did so for the last release but as a change which Afaik doesn't get sent out again

@marcelstoer
Copy link
Member Author

nwf merged commit

Thanks.

I don't know whether this should land on master or release

I see we now have two tags: https://github.com/nodemcu/nodemcu-firmware/releases. We're certainly not going to write release notes twice but which one is it gonna be?

@HHHartmann sorry, I don't understand this remark.

@HHHartmann
Copy link
Member

@marcelstoer When cutting a new release we write a release note which has download links attached. Usually the source code.
To the last release I added, after the release was announced, a luac.cross.exe binary. I would like to add this right on the announcement, so that all receivers of it know that the download exists.
I built the binary locally on my machine, as I did now.
So how can we get this done. I can send it to whoever, as requested.

@marcelstoer
Copy link
Member Author

So how can we get this done.

Attach it to your comment above. Once I get feedback on

I see we now have two tags: https://github.com/nodemcu/nodemcu-firmware/releases. We're certainly not going to write release notes twice but which one is it gonna be?

(from @nwf maybe?) I can write release notes and announce the master drop. Would maybe then also allow us to close #3164?

Side note: just realized that #3191 is still open but should have been "included" before the last master drop.

@nwf
Copy link
Member

nwf commented Sep 22, 2020

I'd go for writing the release notes on the release tag, and maybe add a forwarding link from the master tag citing #3164?

@marcelstoer
Copy link
Member Author

And https://github.com/nodemcu/nodemcu-firmware/#releases needs to be updated, too. I stopped publishing master to RTD and /master redirects to /release: https://nodemcu.readthedocs.io/en/master/

@HHHartmann
Copy link
Member

Since merging this there have been some fixes which might make sense to have in release rather sooner than later.

These are the file fix #3280 , not being able to red files of certain sizes, which I think is essential
and the two enduser_setup PRs #3275 and #3282 would be nice.

BTW: There is also a reference to master in the issue template.

@marcelstoer
Copy link
Member Author

@marcelstoer
Copy link
Member Author

BTW: There is also a reference to master in the issue template.

Sorry, didn't find it. Fortunately GitHub uses the default rather than the master branch:

Issue templates are stored on the repository's default branch

@HHHartmann
Copy link
Member

@HHHartmann you may now attach your binaries to https://github.com/nodemcu/nodemcu-firmware/releases/tag/3.0-release_20200910

Done.
Maybe we can add it before sending out the announcement, so people will more likely spot it.
Should we add this to the docs somewhere?

@HHHartmann
Copy link
Member

HHHartmann commented Sep 29, 2020

BTW: There is also a reference to master in the issue template.
Its here: https://github.com/nodemcu/nodemcu-firmware/blob/release/.github/PULL_REQUEST_TEMPLATE.md

  • This PR is for the dev branch rather than for master.

Changed on dev

@marcelstoer
Copy link
Member Author

marcelstoer commented Sep 29, 2020

Thanks, that'll eventually bubble up to the default branch.

Should we add this to the docs somewhere?

Sure, why not. Maybe by adding a note to a paragraph that talks about lua cross?

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.