Home of the Squeezebox™ & Transporter® network music players.
Page 9 of 12 FirstFirst ... 7891011 ... LastLast
Results 81 to 90 of 120
  1. #81
    Senior Member
    Join Date
    May 2010
    Location
    London, UK
    Posts
    801
    Quote Originally Posted by gordonb3 View Post
    Yes, but that also requires compiling to the correct (ancient) support libraries, although maybe you could get away with static linking on a newer development platform. Of course once you do that you can also fix jive_jiffies() to perform the roll-over itself at some `safe` time within the 2^31 limit.
    Quite easily done with @ralphy's build of Squeezeplay, I think. Well, I know it is on macOS.

    Quote Originally Posted by gordonb3 View Post
    Another option for testing in this case would be to upgrade LUA to 5.4+ and replace all instances of `lua_Number` (signed) to `lua_Integer` (unsigned), which should either shift the restart to after 49 days or possibly continue indefinitely.
    Here I think you get to the nub of it. What might be convenient is a LUA numeric type that wraps around in the same way as the C integer/unsigned type. Then, given that timer setting is limited to 24 days in the future, and that expired timers are checked within 24 days of expiry, the semantics of timer checking can work fine. (One checks the difference between timer value and current time. Just modular arithmetic.) I think both conditions are probably already met in the existing code.

    The LUA numeric type could probably be effectively implemented by defining a suitable LUA function or two, but it looked messy to me. Alternatively perhaps a LUA user defined type might be made to look and act cleaner.


    But, then what ? As far as I am aware, no Radio has ever run longer than 24 days. What else might be revealed ?

  2. #82
    Senior Member Steevee28's Avatar
    Join Date
    Feb 2010
    Location
    Mannheim, Germany
    Posts
    103
    Quote Originally Posted by gordonb3 View Post
    I read that somewhat different, because the values of xxx.expires are created elsewhere from jive_jiffies incremented with some max_process_time value. The sole purpose of those lines you mentioned is thus to identify timeouts.
    I'm sorry. I don't see the point of what you saying here. Being important or not, those timeout checks won't work in the timer-wrapping case, as already pointed out before, and need to be fixed.

    Please also re-visit my post #79. I added further findings of buggy direct timer value comparisons, most of them being part of very central timer handling code in Jive.

    Quote Originally Posted by gordonb3 View Post
    (...)replace all instances of `lua_Number` (signed) to `lua_Integer` (unsigned), which should either shift the restart to after 49 days or possibly continue indefinitely.
    Yes, this should shift the restarts to 49 days. But continue indefinitely? Surely not. AFAIK, for `lua_Number` and `lua_Integer`, the same modular arithmetic rules apply. (EDIT: that's wrong, lua_Number is a C double.)
    Btw, although it feels more "natural" to have unsigned timer values, I would not recommend switching to an unsigned value type, because as also @mrw pointed out, in order to fix the wrapping problems, we need to only deal with timer value differences. And when Lua behaves similar to C/C++, then, the difference between two unsigned values would in turn be unsigned, thus comparing the result to be smaller or greater than zero would be pointless and thus requiring an additional conversion back to a signed value. I already noted this in pseudocode in post #77, `if (signed_value(timer_value - some_point_of_time) > 0) then {do something};`.
    Of course, this could be done, but in order to increase code readability and to prevent programming errors, I'd thus recommend sticking to a signed timer value.

    Quote Originally Posted by mrw View Post
    (...)One checks the difference between timer value and current time. Just modular arithmetic.)(...)
    Yes!!!

    Quote Originally Posted by mrw View Post
    (...)Alternatively perhaps a LUA user defined type might be made to look and act cleaner.(...)
    Yes, as I already mentioned somewhere before, this is exactly what the company I'm working for (as uC-firmware developer...) did, in C++, to prevent code that directly compares timer values and enforce inspecting value differences only. We had timer wrapping in mind when we decided to do so.
    But that might be a step to far here - I don't know, because I know Lua too little and I'm not aware of the amount of code changes required for this here.
    If it can be done with moderate code changes, I'd definitely prefer this solution.

    Quote Originally Posted by mrw View Post
    But, then what ? As far as I am aware, no Radio has ever run longer than 24 days. What else might be revealed ?
    That's a very valid thought that has been discussed before, but IMO, fixing bugs should not be prevented by the fear of finding more bugs ;-)
    Anyway, as you said, a regular scheduled restart might definitely still be a very good idea for Radios.
    We should also keep in mind, that all Jive-based projects (like PiCorePlayer) are affected by this bug.
    Last edited by Steevee28; 2021-03-10 at 10:37. Reason: clarifications, syntax errors
    1x Squeezebox Classic, 3x Radio, 1x Touch, LMS 7.9.1 running on ODROID-U3, Ubuntu 16.04 and I'm happy with it! :)

  3. #83
    Senior Member
    Join Date
    May 2010
    Location
    London, UK
    Posts
    801
    Quote Originally Posted by Steevee28 View Post
    I would not recommend switching to an unsigned value type `lua_Integer`
    The point being, I think, that, at least in C, checking that a signed integer is negative is probably more readable, and possibly more efficient, than checking that an unsigned integer is >= half MAX UINT + 1, or whatever.

    That's a very valid thought that has been discussed before, but IMO, fixing bugs should not be prevented by the fear of finding more bugs ;-)
    Anyway, as you said, a regular scheduled restart might definitely still be a very good idea for Radios.
    We should also keep in mind, that all Jive-based projects (like PiCorePlayer) are affected by this bug.
    I shall put away my fear ! But not a priority at present.

    First step will be to make some reasonable looking proof of concept LUA script function(s) to deal with the LUA side of things. Second step will be to trawl all C and LUA code for occurrences. I don't think that would be too hard.

    One other thing that would need attention (from memory) is the ordering of the timer list.

  4. #84
    Senior Member
    Join Date
    Dec 2020
    Posts
    129
    Quote Originally Posted by Steevee28 View Post
    ... because as also @mrw pointed out, in order to fix the wrapping problems, we need to only deal with timer value differences. And when Lua behaves similar to C/C++, then, the difference between two unsigned values would in turn be unsigned, thus comparing the result to be smaller or greater than zero would be pointless and thus requiring an additional conversion back to a signed value. I already noted this in pseudocode in post #77, `if (signed_value(timer_value - some_point_of_time) > 0) then {do something};`.
    That was my original point, the timer values only being used to calculate a difference in order to detect a timeout situation. I made one wrong assumption though, as you do now, because lua_Number is not an `int` but a `double` (i.e. a floating point number). Meaning that when you increment this value after it was imported as the equivalent of a signed integer it can become a value greater than or equal to 2^31 which the next imported value can never become and therefore the test `t1 + something < t2` with the latter being imported as minus 22 days will always fail. I need to follow the code why this would cause this issue though and moreover why it consistently seems to affect the ui task.

    Edit:
    You're right on one point though. This will undoubtedly cause the same issue if the values are forcibly kept in the positive number range.
    Last edited by gordonb3; 2021-03-10 at 10:24.

  5. #85
    Senior Member
    Join Date
    Dec 2020
    Posts
    129
    Quote Originally Posted by mrw View Post
    The point being, I think, that, at least in C, checking that a signed integer is negative is probably more readable, and possibly more efficient, than checking that an unsigned integer is >= half MAX UINT + 1, or whatever.
    That's actually one of the most efficient operations in computing:
    Code:
    if (value & 0x80000000) {
    ...
    }

  6. #86
    Senior Member Steevee28's Avatar
    Join Date
    Feb 2010
    Location
    Mannheim, Germany
    Posts
    103
    Quote Originally Posted by gordonb3 View Post
    (...) because lua_Number is not an `int` but a `double` (i.e. a floating point number). (...)
    OMG, you're right. I forgot about that (I'm not a Lua programmer). And the other assumption that Lua_Integer is unsigned is also wrong, it is a ptrdiff_t, thus signed. I forgot about all this, although I crawled through all this one year ago:
    see my post #37.

    I believe we are running in circles here...
    1x Squeezebox Classic, 3x Radio, 1x Touch, LMS 7.9.1 running on ODROID-U3, Ubuntu 16.04 and I'm happy with it! :)

  7. #87
    Senior Member
    Join Date
    Dec 2020
    Posts
    129
    Maybe, maybe not. In my experience comparing notes often helps to identify the actual problem a lot quicker.

    Edit:
    Reading your post #37, I guess I missed that one while going over this subject. I'd say my results confirm the suspicion you entered there and the solution will likely be in attacking that bit of code.
    Last edited by gordonb3; 2021-03-10 at 11:31.

  8. #88
    Senior Member
    Join Date
    Dec 2020
    Posts
    129
    Oops! Sort of bricked my Radio by inserting an if clause on `framedue > 0x80000000` which apparently LUA translated into `-1` and thus executed instantly to trigger watchdog. Luckily I was able to recover from it, but obviously it did reset the timer so I'm back to waiting another 24 days to verify what it does.

  9. #89
    Senior Member Steevee28's Avatar
    Join Date
    Feb 2010
    Location
    Mannheim, Germany
    Posts
    103
    Quote Originally Posted by gordonb3 View Post
    (...) but obviously it did reset the timer so I'm back to waiting another 24 days to verify what it does.
    Are you able to edit the jive_jiffies() function in src/common.h to return a timer value right from the start that is - let's say - 1 minute before wrapping (corresponding to 0x7FFF15A0) ?
    That would dramatically speed up testing.
    e.g.
    Code:
    /* time */
    #if HAVE_CLOCK_GETTIME
    static inline Uint32 jive_jiffies(void)
    {
    	struct timespec now;
    
    	clock_gettime(CLOCK_MONOTONIC, &now);
    	return (now.tv_sec*1000)+(now.tv_nsec/1000000) + 0x7FFF15A0;
    }
    #else
    #define jive_jiffies() (SDL_GetTicks() + 0x7FFF15A0)
    #endif
    1x Squeezebox Classic, 3x Radio, 1x Touch, LMS 7.9.1 running on ODROID-U3, Ubuntu 16.04 and I'm happy with it! :)

  10. #90
    Senior Member Steevee28's Avatar
    Join Date
    Feb 2010
    Location
    Mannheim, Germany
    Posts
    103

    Sum-up post required

    Ok guys, I heavily edited my post #79, trying to concentrate all our findings so far, because, as my own previous posts showed, I already forgot about many things that were discussed and identified as problems or possible solutions before.

    So it seems to urgently need a place summing up all these findings.
    I hope that you agree to what I've written there. Please tell me if you don't.

    Is there a way to make post #79 sticky or something like that?

    Is there a way that the post can be edited not only be me?
    (I don't want to be the one and only being able to contribute to that sum-up)
    1x Squeezebox Classic, 3x Radio, 1x Touch, LMS 7.9.1 running on ODROID-U3, Ubuntu 16.04 and I'm happy with it! :)

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •