Questions about the network implementation

Is there a reason the network buffer for sending is so small: https://github.com/espruino/Espruino/blob/master/libs/network/socketserver.c#L218
On the esp8266 that will cause lots of tiny packets, which isn't good. Could I introduce a #define that gets set for different platforms to make this bigger in the esp8266 case?

Why does clientRequestWrite in socketserver.c not kick off any transmission? I suppose the transmission only happens when the idle state is reached? Or am I missing something? Also, I don't understand https://github.com/espruino/Espruino/blob/master/libs/network/socketserver.c#L660, it seems to wipe out anything in the dSnd buffer, meaning that two consecutive calls to write overwrite each other?

NB: maybe "if it ain't broke don't fix it" applies here, but the intermingling of HTTP into plain sockets is not exactly clean. Makes it harder to figure out how to add MQTT or other protocols.

Is there a reason the network buffer for sending is so small:

It's that CC3000 needed a big static buffer to send the data (which I wanted to be as small as possible), but I think on non-CC3000 builds we could change that. If we could be sure the send function was only called from idle (I think it might be?) we'd be safe to allocate quite a large amount of data on the stack for that array.

The 500-odd bytes you talked about as the limit for packets on that ESP8266 build sounds good.

Why does clientRequestWrite in socketserver.c not kick off any transmission?

I think it's because when using GSM or ESP8266 via AT, JavaScript gets called to do the write - and so we don't fill the stack up it makes sense to do that on idle. People also tend to use multiple writes, when realistically you want to send in as few chunks as possible so you want to group writes together.

I don't understand github.com/espruino/Espruino/blo­b/master/libs/network/socketserver.c#L66­0

Yes, that looks wrong - and a potential memory leak. I'm not sure why that hasn't caused problems before... It'd be interesting to see if we could trigger it with some code. I guess an HTTP chunked POST should do it?

the intermingling of HTTP into plain sockets is not exactly clean.

No... My reasoning was:

• There is still quite a lot of duplicated code in each and I was desperately trying to get code size down
  
• I wanted as few lists of sockets in memory as possible, so it made sense to stick them all in together and handle them as one.
  
  

But yeah, it was more of an issue before I abstracted out all the socket stuff, it seems a bit more pointless now.

I think MQTT should still be a JS module, as it currently is - the main addition would be WebSockets and UDP, but I think those should actually fit in quite well.

I don't understand github.com/espruino/Espruino/blo­b/maste­r/libs/network/socketserver.c#L66­0

I just looked at this again, and it's correct.

The else refers to the if (options). What it's saying is (in a roundabout way) is 'if we're an HTTP connection, send a header, else send nothing'.

If sendData is not undefined then we know the header has already been sent, and that if statement has no else on it.

For some reason the indentation there is confusing. I'll add some comments.

I'm trying to reason about plain (non-http) sockets. For plain sockets options is always false. So you always clear sendData (aka httpClientReqVar.dSnd) and that wipes out any data that might be in the socket.

I'll try to trigger it, the logic is not all that clear to me so perhaps I'm just not getting it.

you always clear sendData

Ahh, but that's inside a if (!sendData) statement?

Duh, sorry, the logic got scrambled in my head, you're correct.

It is nasty though. If you get the latest from GitHub I added a few comments.

I know this has been asked and answered elsewhere but I can't find it: how is the socket implementation (e.g. network_esp8266.c) supposed to return errors to the socket library? I know that it can return -1 to a send or recv call to indicate that the socket is closed, but what should it do to tell the application that this is due to an error, and if so, provide an error message?

@tve there's nothing in there for text errors at the moment - you just have to return a negative number, so you could have different numbers for different errors. Maybe a errorNumberToString type function could be added? Or maybe just getLastErrorString is easier - but then it probably requires a string buffer in RAM somewhere.

I'd like to propose the following, lemme know if you're OK with me trying it out:

• define a bunch of negative numeric error codes for socket errors, use -1 as generic/unknown error
  
• update the esp8266 network code to return the appropriate error code
  
• in socketserver.c, when netrecv/netsend/netcreate return an error code enqueue an error callback on the socket or httpCRs object passing the error string corresponding to the code as argument (right now neither error callback takes any argument, so this should be backwards compatible)
  
  

Thoughts?

I just took a peek at the node.js Errors class. In order to keep things simple, I would make the error parameter just a string with an error message and in other places where there is an err parameter the same with the continued convention that err==null => no error. I believe this is simple, reasonably compatible with node.js, and compatible with what Espruino has now.

One thing I noticed is that where an on.('error', function... event is used the node.js docs state explicitly that if no error handler is registered a (fatal) exception is thrown. I believe we should do that too, because right now, I think that code that doesn't use an error handler will just hang on error.

Sounds like a good plan!

Wrt fatal errors - what would that actually mean? Since most of the socket handling is done on idle, if there's an error we can't really do much.

But definitely, if there is no error handler we should at least print the error message. Maybe jsiQueueEvents (or whatever it's called) can return false if no callbacks were found (if it doesn't already).

The plan is good as long as there is an 'error id'-like thing at the beginning of the error message string... Nothing is so frustrating than having to test against the full string... Good practice for that is always to add an id followed by a delimiter followed by the text... last but not least for simple, clear communication (and translation, if so needed). Since it is (unfortunately) just defined as error, there is not much to root for the id to have prefix, such as, for example, E for error... If it is though possible to make the information more purposeful - such as a status info - then such messages could look like:

• I34 Info message text
  
• W03 Warning message text
  
• E20 Error message text
  
  

And even more smart, it would be a return status object... If that is too much a challenge because it comes from the low level (assembler) guts, (all or just) the message could be an object in JSON: 'E20 {type:"e", msg:"error message text"}'... With memory in demand though, 20 (number, any sort of integer) or '20' (or 'E20') is good enough (because 0 or '0' work well in the JS testing if (err) or if (err == 0) (...of course, not everyone likes or is a friend of the conversion ambiguity...)

I'm creating a pull request, which has a slew of changes... Mostly fixes to the esp8266 socket stuff but also adds errors to the standard socketserver, i.e. socket and http classes. When in doubt I looked at node.js, so a socket error event handler receives one parameter, which is an object with a code and a message. I'm pretty happy with the client socket test coverage, but the server test coverage is very thin. So I expect there will be some bugs to sort out. The server side has almost no changes at the socketserver level, so I'm hoping I didn't break anything for non-esp8266 versions.

The open issues I have with all this:

• can anyone run some sanity tests on other platforms?
  
• the http request object error handler is not invoked on HTTP errors (such as a 404), but according to the node.js docs it should be and I think it would be really helpful. Thoughts? I suppose I would set code to an error code for "http error" and then add a status field with the http statusCode? (I have never used node.js and the docs are not explicit about this.)
  
• I have not dealt with printing errors if there is no error handler.
  
• I can not load the MQTT library on the esp8266, it runs out of memory, (I'm not at all suprised), so I'm still expecting to write a C library...
  
  

Phew, it feels like I can see the light at the end of the tunnel...

can anyone run some sanity tests on other platforms?

Actually a really good start would be just make clean;DEBUG=1 make in Linux and then ./espruino --test test123.js - you can grep and there are definitely 1 or two HTTP tests in there. That'd test out the socket server pretty well in a way that's easily debuggable (it also tests for memory leaks).

404

Interesting. If that's what Nodejs does then we should copy that - still seems a bit crazy though. I wonder how you're supposed to get the contents of a webpage that is sent without the standard return code?

It's probably something to leave out of this PR though - we could file an issue for it.

printing errors

No problem... another issue on GitHub?

MQTT library

I'd be really against a C library for MQTT. It's just adding complexity and bulking out the firmware on other devices, where space is getting pretty tight now. If you're going to implement something resembling full MQTT it'll take a lot of space - most of which will be for the API, and very little of which will be actual code since the protocol is pretty simple.

... but if you're not trying to be full-featured, why not just make a cut-down JS version? Originally, I'd posted some example code that published data with MQTT and used just a few vars.

Sure it wasn't full-featured, but it worked for what it was made for, and it'd definitely be fine on ESP8266.

If you just wanted to publish (or just wanted to subscribe) you could probably get the code size down significantly.

About the MQTT: I don't know why it would use a lot of space and it oculd easily be part of the SAVE_ON_FLASH ifdef stuff. It's not like the JS module wouldn't be available to those not wanting the C version. I already adapted a pretty complete implementation for the esp8266 and it didn't seem like that much code. What I want is to use MQTT as the main communication protocol for my nodes, so I don't want a half-assed publish-only and don't-check-errors thing. I haven't looked at the Espruino library, but I know it doesn't fit on its own without any other code. I'll take a look at what can be cut down, but that wasn't a hopeful start...

I'd hope things would have improved with the recent changes to Espruino's variable storage, the minified version doesn't look too big, although:

• adding var f=String.fromCharCode at the start could save you a bunch
  
• looks like some things like mqttPacketLength are public. If they weren't their names would get nicely minified.
  
  

How much is 'that much'? If it's even 10k, that's kind of hard to justify on other boards where MQTT works fine at the moment, and where on the original Espruino I'm starting to have to make space-saving changes just to get releases out the door.

If you really want to include a USE_MQTT kind of thing for ESP8266 it's Open Source so is up to you at the end of the day - but honestly it's unlikely to get included in Espruino board builds. It's also very unlikely to get any bugfixes/contributions from anyone - the JS code tends to get users debugging and fixing problems in it, but C code puts a lot of people off :)

... also right now any constant data (including the symbol table?) will use up RAM, so the MQTT lib will still be eating into available RAM a little all the time on ESP8266.

Having said all that - @JumJum was asking about how contributed code could get built in on-demand. Maybe it's worth trying to come up with a more flexible way of handling that.

Just did some tweaks to MQTT, and got the code size down by around 15%. It's now 3.9k minified so I'd be pretty surprised if it didn't fit in.

I've been trying to hack the MQTT module myself over the last couple of days. To get it to load with no "out of memory" I had to be sub 3k. Even then, there was not enough memory to do anything, so the best I achieved was connecting. However, going bottom up and with the help of your Kickstarter example I have a module that will do publish and subscribe with events to sort of match the existing module's API, that comes in 1.3k minified. I'm testing it now and will share it ASAP.

Wow, that'd great! That'd be a huge help!

@Ollie: cool, I'd be glad to try it out!

Gordon, I don't really understand why you're so hostile to non-JS additions. I was going to see whether I can insert a C-level sockets API so protocols in C could go through the Espruino sockets layer and thereby be target independent. I really don't see the issue of having more features available on targets that have more flash or RAM and less on others. This wouldn't affect whether Espruino still fits into the original Espruino board.

I looked again at the JS MQTT module and noticed:

      else if(type === TYPE.PUBACK) {
        // implement puback
      }
      else if(type === TYPE.SUBACK) {
        // implement suback
      }
      else if(type === TYPE.UNSUBACK) {
        // implement unsuback
      }

which tells me that it doesn't even implement QoS 1.

It's here - https://github.com/olliephillips/tinyMQTT
There is a lot missing from the original module, but I've been testing against test.mosquitto.org and it seems to do enough for my use case. If it can be better, and still small, please let me know :)

@Ollie, thanks! That looks a lot smaller! If you're trying to get Espruino memory usage down, you might find that:

function onData(data) {
			if((data.charCodeAt(0) >> 4) === 3) {
				var cmd = data.charCodeAt(0);
				var var_len = data.charCodeAt(2) << 8 | data.charCodeAt(3);
				var msg = {
					topic: data.substr(4, var_len),
					message: data.substr(4+var_len, (data.charCodeAt(1))-var_len),
					dup: (cmd & 0b00001000) >> 3,
					qos: (cmd & 0b00000110) >> 1,
					retain: cmd & 0b00000001
				};
				this.emit('message', msg);
			}
		}

MQTT.prototype.connect = function(){
	var onConnected = function() {
		client.write(mq.mqttConnect(getSerial()));
		mq.emit("connected");
		client.on('data', onData.bind(mq));
// ...

actually uses even less memory on Espruino at the moment (it won't have two copies of the function text in memory).

@tve I'm just trying to make sure that things are as useful to as many people as possible. So few people want to compile their own firmwares, doing stuff in JS lets them choose how they use their memory.

Also the past few years have shown that JS code actually gets many more improvements and tweaks than the C code does - and from my point of view it's good to do things that make it easier for others to contribute.

But if you do add MQTT in C code, feel free to stick it back in the Espruino repo (as long as the licence for the stuff you're adding is compatible with MPLv2). As you say, it could be useful for people that want to build it in.

I was going to see whether I can insert a C-level sockets API so protocols in C could go through the Espruino sockets layer and thereby be target independent.

While that'd be cool, you might find that you have a lot of trouble. Both CC3000 and WIZnet have C-ish socket APIs (each slightly different, but with send/recv/etc), that conflict so they can't even be compiled into the same binary. I'd imagine adding another socket API on top might cause similar problems - on CC3k, WIZnet and Linux.

Great stuff thanks @Gordon I'll try that!

No problem :) Looking at it, you might find some of the tricks I did here:

• https://github.com/espruino/EspruinoDocs/commit/6b9f074690841d5e3a22c5327636be341ffed268
  
• https://github.com/espruino/EspruinoDocs/commit/ded33c61e6570d7fd8deea5e091e86abae5521b0
  
  

Let it minify down even more (if you don't care about exporting mqttPacket/etc). Ignore the git commit explanation - I got confused between that and the WebSockets commit I did at the same time :)