Tracking down a commit causing Serial print to "block"

BennehBoy
Sun Dec 16, 2018 6:40 pm
I just did a git pull against Roger’s core, I do this periodically to check that all my code still works.

It stopped working. Only fails though when doing some serial comms (at 10400 baud) to another stm32 acting as a pretend ECU.

So, I did a git reflog to see what the previous pull hash was and found it to be: 4db3994 HEAD@{1}

So, after doing: git reset –hard HEAD@{1}

I rebuilt and the issue resolved.

I had a quick look through the commit history and can’t see that ID so suspect it’s a sub ID of some sort, anyhow my question is, how can I list just the commits between that hash, and the current latest which is 60d982f ?


BennehBoy
Sun Dec 16, 2018 6:43 pm
PS, this manifests as a hard hang.

mrburnette
Sun Dec 16, 2018 7:06 pm
I rarely do a git pull…

Rather, I prefer to be Joe-user and just download the ZIP and unZip to a temp directory. Then I use the Linux utility MELD to do a directory, file-by-file diff. I like the pretty colors and the column/column presentation seems to fit my particular way of thinking.

Ray


BennehBoy
Sun Dec 16, 2018 7:34 pm
Well I’ve figured it out, turns out that I must’ve not pulled it as recently as I had thought….

Just need to figure out where/why/what I need to change…

commit 60d982ff25d966f15e8af70d4bb6f46bb1d1595a
Merge: c705499 99b9aca
Author: Roger Clark <[email protected]>
Date: Mon Dec 3 09:01:31 2018 +1100

Merge pull request #580 from Squonk42/titles

Explicit STM32duino.com platform origin

commit 99b9acaa80d2394314c7668ff83a4953104cec31
Author: Michel Stempin <[email protected]>
Date: Sun Dec 2 22:32:30 2018 +0100

Explicit STM32duino.com platform origin

commit c7054992554b27ec6bdfc016347806454ab69815
Author: Roger Clark <[email protected]>
Date: Fri Nov 30 20:19:24 2018 +1100

Removed LTO options as the no longer work correctly with the newer version of gcc used in Arduino 1.8.7

commit 9a489ca25fa2794bd39d4d2d46e1d9d3ecd733e2
Author: Roger Clark <[email protected]>
Date: Sun Nov 11 20:38:47 2018 +1100

Changed USB Serial so that it will send to the host even if DTR is not set. Note this does not effect the operation of the boolean operation e.g.. while(!Serial); used to wait for the Arduino terminal to open

commit 61ed869caa6bf73a4ca28e325e200670fc8ef8e5
Merge: b5cd376 188d9ce
Author: Roger Clark <[email protected]>
Date: Mon Sep 24 17:35:30 2018 +1000

Merge pull request #569 from stevstrong/patch-20

F4: fix HID upload with Arduino IDE

commit 188d9ce985678f72f4912cc742ee3865f92dfd7b
Author: stevstrong <[email protected]>
Date: Mon Sep 24 09:31:55 2018 +0200

F4: fix HID upload with Arduino IDE

commit b5cd37696b7057f8489c0a17801420756b44a4e7
Author: Roger Clark <[email protected]>
Date: Wed Sep 12 17:22:11 2018 +1000

Updated USBComposite library. Thanks to @arpruss

commit 6a9c7956538892fa6fbe4a9ffc90d3c4bfab9441
Merge: b31efe8 92cdd1a
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 17:23:01 2018 +1000

Merge branch 'stevstrong-patch-16'

commit 92cdd1ab217c50e47c87268cfbc95e128dbc2591
Merge: b31efe8 3d356aa
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 17:21:34 2018 +1000

Merge branch 'patch-16' of https://github.com/stevstrong/Arduino_STM32 into stevstrong-patch-16

commit b31efe83f743f234524969bab4a26e02b55b27f6
Merge: e65747d 32d2eab
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 17:02:18 2018 +1000

Merge branch 'smarq8-master'

commit 32d2eabb608d0f2006d872a6017326c4a1037aba
Merge: e65747d f781964
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 16:19:57 2018 +1000

Merge branch 'master' of https://github.com/smarq8/Arduino_STM32 into smarq8-master

commit e65747d8bbd8af80ff08decd2f1a564f7ae93d85
Merge: 231b9d3 e245726
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 15:59:47 2018 +1000

Merge branch 'fpistm-default_build_flag'

commit e245726117532897ff2394c8dff4352046b1878e
Merge: 231b9d3 e160160
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 15:53:21 2018 +1000

Manually merged conflict with hid upload pattern in platform.text

commit 231b9d350cea8e2f6e92556c2bf02789b81c4d70
Merge: 6e2796e 7f487b3
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 15:48:33 2018 +1000

Merge branch 'pamribeirox-master'

commit 7f487b33c60affea8a5243d631f8734bcd7615e5
Merge: 6e2796e 129e57b
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 15:43:20 2018 +1000

Merge branch 'master' of https://github.com/pamribeirox/Arduino_STM32 into pamribeirox-master

commit 6e2796e93893a1521fb3b9b567d4400edb500bdb
Merge: f4cc34b a176fd1
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 15:00:09 2018 +1000

Merge pull request #547 from fpistm/Fix_netduino2plus

[STM32F4] Add missing build.vect_flags for netduino2plus

commit f4cc34b82808bb5e10e90bfe3fd9dddd8d1adaab
Merge: 45439cf 9be1115
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 14:58:12 2018 +1000

Merge branch 'tfry-git-master'

commit 9be1115f03ff71d353ba9662a9838f4d1bf5ac4b
Merge: 45439cf 3cb1196
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 14:57:18 2018 +1000

Merge branch 'master' of https://github.com/tfry-git/Arduino_STM32 into tfry-git-master

commit 45439cf42d30350fbbf46f96a8849e42b6880e6d
Merge: 26ae0cc 9870a47
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 14:56:19 2018 +1000

Merge pull request #549 from stevstrong/patch-17

F4: USBD_StrDesc[] size fix

commit 26ae0cc7de3296bea44c5f524ca963cc1c8f4525
Merge: 8a34f8e 3ff3b0a
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 14:55:56 2018 +1000

Merge pull request #550 from stevstrong/patch-18

F4: USBD_StrDesc[] size fix (continued)

commit 8a34f8e2214b674d976fa8e2b9216fa91d0706d0
Merge: 9d46a1c 4ea490e
Author: Roger Clark <[email protected]>
Date: Sun Aug 19 14:51:06 2018 +1000

Merge branch 'Serasidis-master'

commit 4ea490e6d377980012aed198c3fbaabbf94c8d6f
Author: Vassilis Serasidis <[email protected]>
Date: Sat Aug 4 17:29:51 2018 +0300

Added support to HID Bootloader 2.0

HID Bootloader 2.0 supports Bluepill and generic_f407v boards
The hid-flash tool is at the moment available only on Windows

Please update your HID Bootloader firmware to V2.0
(https://github.com/Serasidis/STM32_HID_Bootloader/tree/master/bootloader/bootloader_only_binaries)

commit 3ff3b0aef3fa88c19755b9e873243e89c45d9bb4
Author: stevstrong <[email protected]>
Date: Sat Aug 4 10:51:21 2018 +0200

F4: USBD_StrDesc[] size fix (continued)

Limit the maximum string length to 32 chars.

commit 9870a471db765c08dbfbedfae69794dea7742962
Author: stevstrong <[email protected]>
Date: Fri Aug 3 18:01:02 2018 +0200

F4: USBD_StrDesc[] size fix

- bug: memory overflow for size values less than 62, in function USBD_GetString().
The array in question (USBD_StrDesc) is followed in memory by 2 fill bytes for the current compile order.
Arduino 1.8.6 compiles the files in different order, so that the USB will not working anymore.
The longest string is 32 bytes long (plus ending zero), and 2 more bytes will be added at the beginning (https://github.com/rogerclarkmelbourne/Arduino_STM32/blob/master/STM32F4/cores/maple/libmaple/usbF4/STM32_USB_Device_Library/Core/src/usbd_req.c#L818-L834).
The last 2 bytes are fill bytes (align 4).

commit 3cb1196851088fcef5fa89d14841158a6e7f7d02
Author: Thomas Friedrichsmeier <[email protected]>
Date: Thu Aug 2 20:47:25 2018 +0200

Add pgm_read_ptr and pgm_read_ptr_far for better compatibility with libs written for AVR

commit a176fd1937faba29fadc4df6907037b45bf451ab
Author: Frederic.Pillon <[email protected]>
Date: Tue Jul 31 16:07:11 2018 +0200

[STM32F4] Add missing build.vect_flags for netduino2plus

Broken since:
58e50e7fd9062e45ec23d575a16cd411522c37cb

Fix #544

Signed-off-by: Frederic.Pillon <[email protected]>

commit 129e57b854e7d6674a6ddc28c7c8ecaa83f3ab8c
Author: Pedro Ribeiro <[email protected]>
Date: Mon Jul 30 22:27:19 2018 +0100

Revert the previous proposed changes

commit 53612be98f05462894acdafc8ba45eae7127b660
Author: Pedro Ribeiro <[email protected]>
Date: Mon Jul 30 22:26:46 2018 +0100

Revert the previous proposed changes

commit 9b927b2f47a25ad9878f70c58f0a2f6495a3965e
Author: Pedro Ribeiro <[email protected]>
Date: Mon Jul 30 22:18:56 2018 +0100

Fix I2C handling of multiple Wire.begin() at libmaple level

Just skip the i2c_master_enable() code if the PE bit says it's already enabled
This solves the problem of ASSERT() hang when Wire.begin() is called by multiple libraries

commit 91783ae6b564c822915651c57096b99ed8b60948
Author: Pedro Ribeiro <[email protected]>
Date: Sun Jul 29 20:51:44 2018 +0100

Avoid i2c_master_enable() multiple calls

Repeated calls seem to trigger some low level ASSERT()
We can't avoid begin() being called by each library from the devices sharing the bus
Use the attribute isbegin to keep track of begin() calls and does the i2c_master_enable() at the first one for the object instance

commit 65e4bfd2a89a8dfabfee2c5b58ef0cd935c4701f
Author: Pedro Ribeiro <[email protected]>
Date: Sun Jul 29 20:37:00 2018 +0100

New attribute "isbegin"

This attribute is added to keep record o previous begin() calls to the instance.
Repeated calls seem to trigger some low level ASSERT()
We can't avoid begin() beeing called by each library from the devices sharing the bus

commit e160160034004a2e7019b84d60babccd14d8661e
Author: Frederic.Pillon <[email protected]>
Date: Thu Jul 26 17:46:23 2018 +0200

Define defaults optimization build.flags

Signed-off-by: Frederic.Pillon <[email protected]>

commit f7819640932d921ef5cbee26976e54770d7df231
Author: smarq8 <[email protected]>
Date: Fri Jul 20 15:57:10 2018 +0200

Update HardwareSerial.cpp

fix availableforwrite()

commit 3d356aaf4578bfdb9af1881dece617374d05fa6a
Author: stevstrong <[email protected]>
Date: Fri Jul 6 12:15:03 2018 +0200

F1: fix wrong delay timing

commit d4b3cd114ba567dbd917b7373f2160d14dd29fd4
Merge: 9d46a1c 2ae1847
Author: Roger Clark <[email protected]>
Date: Mon Jul 2 19:51:54 2018 +1000

Merge pull request #534 from stevstrong/patch-16

F1: added function dma_get_count

commit 2ae184754b387f67dd7647fc620ea9ccac152eff
Author: stevstrong <[email protected]>
Date: Mon Jul 2 10:32:21 2018 +0200

Update dma.h

small correction

commit 8050dbfa580b97bf375b8f6f83894c4c2ce6478e
Author: stevstrong <[email protected]>
Date: Mon Jul 2 10:30:17 2018 +0200

F1: added function dma_get_count

commit 9d46a1c27d4dc9dd4df06b76a1d81684519d8acc
Author: Roger Clark <[email protected]>
Date: Sat Jun 23 17:00:17 2018 +1000

Alternative fix for issue #532

commit 683b6d27969b5b423b881fb594cce1c04a116ce5
Merge: ffce10c 2f4eaa7
Author: Roger Clark <[email protected]>
Date: Sat Jun 23 16:51:55 2018 +1000

Merge pull request #533 from Serasidis/master

STM32F407 BKPSRAM Boundary address fix

commit ffce10ce29f0915eadaeef44a66af68bfd6e10b2
Merge: 4db3994 e8d8cdb
Author: Roger Clark <[email protected]>
Date: Sat Jun 23 16:51:12 2018 +1000

Merge branch 'arpruss-master'

Initial commit


BennehBoy
Sun Dec 16, 2018 7:46 pm
OK, so this is now a bit strange….

My sketch is outputting debug info to Serial (USBSerial), and sending/receiving OBDII requests via Serial3.

If I don’t have the Serial Monitor open the sketch hangs, I assume when, the buffer is full, if the monitor is open then all is fine.

There are a couple of commits that could be at cause (in my mind):
commit 9a489ca25fa2794bd39d4d2d46e1d9d3ecd733e2
Author: Roger Clark <[email protected]>
Date: Sun Nov 11 20:38:47 2018 +1100

Changed USB Serial so that it will send to the host even if DTR is not set. Note this does not effect the operation of the boolean operation e.g.. while(!Serial); used to wait for the Arduino terminal to open


fpiSTM
Mon Dec 17, 2018 7:04 am
Maybe this is what you are looking for
git log --pretty=oneline 4db3994..HEAD

stevestrong
Fri Jan 18, 2019 9:19 am
I acknowledge, the USB serial hangs as long as data is not taken by the host.
There is a discussion on github regarding this issue: https://github.com/rogerclarkmelbourne/ … d3ecd733e2

RogerClark
Fri Jan 18, 2019 8:46 pm
This was an unintended side effect of a change to fix another problem.

Ideally USB Serial should be able to send data to host software that does not set DTR, and it seems that the Arduino IDE is in the minority of programs which set DTR by default

But this is a major bug that I need to fix and the quickest thing I can do is to put things back to the way there were, and then try to work out a solution which allows data to be sent when DTR is not set, but does not block when there is no host to receive the data etc


mrburnette
Sat Jan 19, 2019 2:30 am
[RogerClark – Fri Jan 18, 2019 8:46 pm] –

Ideally USB Serial should be able to send data to host software that does not set DTR, and it seems that the Arduino IDE is in the minority of programs which set DTR by default

I read the github messages; as the forum historian, I just want to remind everyone that the LeafLabs version of the Arduino IDE was their own distribution of V0022… so, they could do what the heck they wanted.

My plea is to just fix things and leave the core alone moving forward so that the testers have a stable base to do their testing. The core cannot be a moving target as testing is ongoing. IMO, it is better to have issues and work-around documentation than to implement changes and then roll-back to a previous version.

Ray


ag123
Sat Jan 19, 2019 4:11 am
i think steve is right about one thing
https://github.com/rogerclarkmelbourne/ … t-31985859
stevstrong wrote:
The most Arduino boards will work without DTR because they have USART serial.

When I fixed the USB serial problem for F4, I analyzed the complete USB serial protocol and had to come to the conclusion that, AFAIK, in case of USB serial , DTR is the only flag available for host application to let the client know that he can receive data. So I don’t think it was a mistake from Leaflabs to use it.

Extract from wikipedia:
“Data Terminal Ready (DTR) is a control signal in RS-232 serial communications, transmitted from data terminal equipment (DTE), such as a computer, to data communications equipment (DCE), for example a modem, to indicate that the terminal is ready for communications and the modem may initiate a communications channel.”

It is true, we have here USB serial instead of RS232, but the scope is the same.


RogerClark
Sat Jan 19, 2019 11:39 pm
I think the ideal solution would be for me to fix the issue, which is being caused by Serial.Print() blocking, because the buffer gets full when it can’t send to a host.
However, I could accidently introduce a bug by trying to fix this.

So since producing a stable release seems to be everyone’s priority, the best thing I can do is simple swap the ifdef to ifndef (or vice versa), so that this change is off by default.

Then I’ll probably change the “development” branch to add that change back in again, and try to fix its consequences.

Its interesting how what I thought would be a simple change, caused a problem which does not effect the majority of people.
Hence even automated testing e.g. to check the code compiles, and even runtime testing, would probably not have detected this bug.


mrburnette
Sun Jan 20, 2019 3:38 am
[RogerClark – Sat Jan 19, 2019 11:39 pm] –

Its interesting how what I thought would be a simple change, caused a problem which does not effect the majority of people.
Hence even automated testing e.g. to check the code compiles, and even runtime testing, would probably not have detected this bug.

Having started one QA group (software) and having managed a group of 10 JAVA programmers for what seemed forever, I have seen your story repeated many times. One of the biggest challenges in QA is creating test suites and representative data (inc. metadata) suites. This is half art and half magic.

I agree with you that the (this) issue would likely have slipped through the test mill; but, it is representative of the need to perform lengthy and wide testing – assuredly I do not see this core (Roger’s core) as being any better than the LeafLab’s core… which they wrote, nurtured, and abandoned. The initial code appears to be the primary work of one individual to meet a business need, but once out in the wild, the cost to continue the evolution drove the business to shutdown the product line.

Ray


RogerClark
Sun Jan 27, 2019 4:21 am
I’ve spent some time looking at this issue and its not as problematic as I first thought.

I initially presumed that if you used USB serial for debugging, and then deployed it somewhere its not connected to a computer (which would probably be the norm), that USB Serial would block.

But it doesn’t block if you run from a external power source or a USB charger etc.

The problem only occurs if you are connected to a computer (e.g. PC) which enumerates the USB bus, but you don’t have a serial terminal running to accept the incoming data.

Anyway.

Looking at the poll from about a year ago…

https://www.stm32duino.com/viewtopic.php?t=2477

Most people wanted the choice about whether USB Serial should be blocking, and second in the poll as that USB Serial should not block.

So…
What I propose is, to Not Block by default, as I think this overcomes the issue highlighted in this thread
But also to add the functions to enable and disable blocking as referenced my Martin

I know this is a change to the core and won’t please everyone as it may break some existing code, but fixing things where the libmaple core does not adhere to the Arduino API (or behave like most Arduino AVR boards do), is IMHO a good idea, even if it breaks 1% of existing applications.

<flame protection helmet on>


RogerClark
Sun Jan 27, 2019 8:30 am
FYI

My new code that appears to work (in usb_serial.cpp)

size_t USBSerial::write(const uint8 *buf, uint32 len)
{
size_t n = 0;

#ifdef USB_SERIAL_REQUIRE_DTR
if (!(bool) *this || !buf) {
return 0;
}
#else
if (!buf || !(usb_is_connected(USBLIB) && usb_is_configured(USBLIB))) {
return 0;
}
#endif

uint32 txed = 0;
if (!_isBlocking) {
return usb_cdcacm_tx((const uint8*)buf + txed, len - txed);
}
else {
while (txed < len) {
txed += usb_cdcacm_tx((const uint8*)buf + txed, len - txed);
}
}

return n;
}


BennehBoy
Sun Jan 27, 2019 9:32 am
Roger, to be honest if it doesn’t block whilst USB is disconnected that’s good enough for me.

I only noticed this whilst developing against my project and the boards are always USB powered at those times, hence DTR would make a difference.


Leave a Reply

Your email address will not be published. Required fields are marked *